fix(datetime): reinit behavior on presentation change #24828
Conversation
|
I am noticing the following error getting thrown during the E2E tests: TypeError: Cannot read properties of undefined (reading 'initializeCalendarIOListeners')
at initializeListeners (ion-datetime.entry.js:1659)
at Datetime.componentDidRender (ion-datetime.entry.js:1749)
at safeCall (index-765cc965.js:1580)
at postUpdateComponent (index-765cc965.js:1481)
at postUpdate (index-765cc965.js:1416) undefinedPerhaps this is why the test is failing? |
|
I'm still working on it; there are a few different issues that need addressing. I'll re-request review when it's ready again. |
| * purposes, e.g. to ensure month selection works. | ||
| * @internal | ||
| */ | ||
| @Event() ionWorkingPartsDidChange!: EventEmitter<DatetimeParts>; |
There was a problem hiding this comment.
This should replace the InitMonthDidChangeEvent function completely. I'll make a separate PR to update the other tests once this has cleared review.
There was a problem hiding this comment.
Is there a way we can have a check that is only done in tests? My concern with this is that this extra code is only needed for tests but is now going to run in every production Ionic app that uses ion-datetime.
There was a problem hiding this comment.
I wasn't able to find one, but I'm open to ideas. The old util (InitMonthDidChangeEvent) fits the bill and uses a mutation observer to indirectly watch for the month to change, but it doesn't account for the month element being re-rendered (and thus the reference to the observed element being lost) when presentation is changed.
There was a problem hiding this comment.
Spent some time exploring an alternative way to test this behavior. Here's a branch that shows how we can drop the internal event from ion-datetime: https://github.com/ionic-team/ionic-framework/tree/test/FW-844.
It will apply a mutation observer to dispatch a custom event once the calendar body is manipulated. It will also re-attach the observer to the correct element node after the presentation is changed.
If you like this approach, feel free to take any parts of it/merge it into your working branch.
There was a problem hiding this comment.
@sean-perkins Thanks for that! I cleaned it up and brought it in 👍 cc @liamdebeasi
|
Alright, should finally be good to go now! Whew. |
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
When the
presentationprop ondatetimeis changed programmatically, behaviors such as the intersection observers and keyboard listeners are disconnected.Issue URL: Resolves #24826
What is the new behavior?
calendarBodyRefuses were updated to avoid being set tonulldue to a Stencil bug.Does this introduce a breaking change?
Other information