fix(datetime): changing months work if partially visible - #27917
Conversation
|
|
|
Found an edge case. Moving back to draft for now. |
| const datetime = page.locator('ion-datetime'); | ||
| const monthYearToggle = page.locator('ion-datetime .calendar-month-year'); | ||
| const monthColumnItems = page.locator('ion-datetime .month-column .picker-item:not(.picker-item-empty)'); | ||
| const ionChange = await page.spyOnEvent('ionChange'); |
There was a problem hiding this comment.
This test started to flake after my changes. This test touches the wheel picker which does not use any of the changed code. I noticed that the test was not waiting for .datetime-ready and it was not waiting for the ionChange event, so it's possible that my change made datetime slightly faster/slower such that this test is now consistently failing.
I simplified the test but made sure to check the same datetime APIs, so this behavior is definitely still working.
| await monthColumn.locator('.picker-item[data-value="10"]').click(); | ||
| await page.waitForChanges(); | ||
|
|
||
| await yearColumn.locator('.picker-item[data-value="2021"]').click(); |
There was a problem hiding this comment.
This ended up being redundant because clicking "October" automatically switched the year to "2021"
|
|
||
| await yearColumn.locator('.picker-item[data-value="2021"]').click(); | ||
| await page.waitForChanges(); | ||
| await ionChange.next(); |
There was a problem hiding this comment.
This test also started to fail only in Firefox. The problem was that tapping the "October" button started to scroll, but we closed the month/year picker before the scroll finished so the value was never updated. By waiting for ionChange we can ensure the value is updated.
averyrousseau
left a comment
averyrousseau
left a comment
There was a problem hiding this comment.
Nice work, this should make things way more efficient. Just had one quick question about a test but otherwise LGTM.
|
Will this fix make it to version 6? Thanks |
|
No, this fix is only for Ionic 7. We are not actively developing Ionic 6 anymore. |
Issue number: resolves #29198 --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> In #27917 I failed to account for RTL. This caused the navigation with the arrow buttons to break because the scroll position was always < 2. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - Datetime now checks if the component is in RTL mode when creating the scrollLeft threshold. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. -->
4 participants
Issue number: resolves #27913
What is the current behavior?
When determining what the changed month is, we grab the element at the center of the datetime and then grab the nearest calendar month. This works fine if the datetime is fully in view, but if the center point is out of the viewport then this will return
null. As a result, scrolling in the datetime will break.What is the new behavior?
Does this introduce a breaking change?
Other information
Dev build:
7.2.2-dev.11690996559.1019674a