Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
| ); | ||
| if (dropdownContent) this._dialog = true; | ||
|
|
||
| this.addEventListener('blur', () => this._isInitialFocusDate = true); |
There was a problem hiding this comment.
Interesting... so just the fact that these event handlers were being wired up in firstUpdated was somehow causing an extra update cycle?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| firstUpdated() { | ||
| super.firstUpdated(); | ||
| this.removeEventListener('blur', this._onBlur.bind(this)); |
There was a problem hiding this comment.
Are these supposed to be adding the handlers instead?
|
🎉 This PR is included in version 3.227.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
GAUD-9526