Skip to content

fix: remove update cycle from calendar #6633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
GZolla wants to merge 1 commit into
base: main
Choose a base branch
from
+20 −9
Open

fix: remove update cycle from calendar #6633

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions components/calendar/calendar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,11 +455,12 @@ class Calendar extends LocalizeCoreElement(LitElement) {
this._monthNav = 'initial';
this._namespace = 'components.calendar';
this._tableInfoId = getUniqueId();
this._onBlur = this._onBlur.bind(this);
this._onLocalizeResourcesChange = this._onLocalizeResourcesChange.bind(this);
getCalendarData();
}

firstUpdated(changedProperties) {
super.firstUpdated(changedProperties);
connectedCallback() {
super.connectedCallback();

if (this.minValue && this.maxValue && (getDateFromISODate(this.minValue).getTime() > getDateFromISODate(this.maxValue).getTime())) {
throw new RangeError('d2l-calendar component expects min-value to be before max-value');
Expand All @@ -471,17 +472,18 @@ class Calendar extends LocalizeCoreElement(LitElement) {
);
if (dropdownContent) this._dialog = true;

this.addEventListener('blur', () => this._isInitialFocusDate = true);
Copy link
Member

@dlockhart dlockhart Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... so just the fact that these event handlers were being wired up in firstUpdated was somehow causing an extra update cycle?

Copy link
Contributor Author

@GZolla GZolla Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were not, the rest of the firstUpdate code was, so we could keep them on firstUpdated but it would be the only thing there. And anyways, I believe this fits better with the recommended pattern for lit event handling.

The examples on that page define internal listeners on the constructor but does not explicitly recommend that approach. It does however explicitly recommend that external listeners (e.g. for window or other components) would be defined on connectedCallback, so might as well define internal listeners here too.

Copy link
Contributor

@dbatiste dbatiste Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we have some best practices for this in the Daylight docs with explanation which I believe aligns with the Lit documentation.

While handlers on this can be managed from connected/disconnectedCallback, wiring up in that lifecycle method really just preferred when wiring up handlers to external elements. Wiring up to this in firstUpdated or the constructor simplifies managing it. There's the perf things too, but I think it is probably negligible.


this.addEventListener('d2l-localize-resources-change', () => {
getCalendarData();
this.requestUpdate();
});
this.addEventListener('blur', this._onBlur);
this.addEventListener('d2l-localize-resources-change', this._onLocalizeResourcesChange);

this._today = getDateFromDateObj(getToday());
if (this.selectedValue) this._getInitialFocusDate();
else this.reset();
}

disconnectedCallback() {
super.disconnectedCallback();
this.removeEventListener('blur', this._onBlur);
this.removeEventListener('d2l-localize-resources-change', this._onLocalizeResourcesChange);
}

render() {
Expand Down Expand Up @@ -686,6 +688,10 @@ class Calendar extends LocalizeCoreElement(LitElement) {
this._shownMonth = getNextMonth(this._shownMonth);
}

_onBlur() {
this._isInitialFocusDate = true;
}

async _onDateSelected(e) {
let selectedDate = e.composedPath()[0];
if (selectedDate.tagName === 'BUTTON') selectedDate = selectedDate.parentNode;
Expand Down Expand Up @@ -866,6 +872,11 @@ class Calendar extends LocalizeCoreElement(LitElement) {
await this._showFocusDateMonth(oldFocusDate, Math.abs(numDaysChange) !== 1);
}

_onLocalizeResourcesChange() {
getCalendarData();
this.requestUpdate();
}

async _onNextMonthButtonClick() {
this._monthIncrease();
this._triggerMonthChangeAnimations(true);
Expand Down
Loading