adobe
fix useRangeCalendar behavior while interacting outside
Closes #5703, #5709
This is a proposal to fix useRangeCalendar behavior by directly showcasing what was wrong (IMO).
Most problem were state on #5703. But I just can't state the issue any better. This PR would hopefully make things more clear.
✅ Pull Request Checklist:
- [x] Included link to corresponding React Spectrum GitHub Issue.
- [x] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
- [x] Filled out test instructions.
- [ ] Updated documentation (if it already exists for this component).
- [ ] Looked at the Accessibility Practices for this feature - Aria Practices
📝 Test Instructions:
The videos is recorded with existing story.
The main difference is instead of commit selection, I canceled it. And canceling means that onChange would not get called.
| Difference | Original | My Proposal |
|---|---|---|
| Click on empty cell, td, title of calendar or anywhere outside | ||
| Scrolling start on empty cell, td, title of calendar (or anywhere outside the calendar) should not affect selection | ||
| Drag onto empty cell, td, title of calendar or anywhere outside |
| Remain the same | Original | My Proposal |
|---|---|---|
| Scrolling start on a CalendarCell | ||
| Change viewport by navigation buttons | ||
| Select range by dragging |
🧢 Your Project:
https://github.com/QzCurious/react-spectrum
I don't know why but it seems like there were a few test case failed on commits before my.
I've signed Adobe CLA and made CI running. Did "updated behavior" mean the code I submitted? If so, could you point me which test cases should I take care of?
It's currently too many tests failed on my machine (node v18.18.2 on commit ee51290b9).
I'm not sure these test cases were "correctly failing" or caused by my environment.
@QzCurious Could you try node version v18.17.1? Some versions of node had a breaking change to Date/Time formatting, might be what you are running into here
It works on v18.12.1 rather.
The relevant failed cases are:
- releasing drag outside calendar commits it
- clicking outside calendar commits selection
- selects the nearest available date when blurring the calendar
And I've changed those to match new behavior:
- cancel ongoing selection when releasing drag outside calendar
- cancel ongoing selection when clicking outside calendar
- cancel ongoing selection when blurring the calendar
By the way, v18.17.1 works on a windows machine.
On my mac book, there are other failed tests. Seems like some were caused by encoding. And some might be platform specific.
The output text here
Colored output here
Build successful! 🎉
Hi, just wanted to know how it goes. Let me know if there's anything I can help with.
@QzCurious apologies, we've got a bit side tracked with last release, I'll see if we can get this into an upcoming testing session for the team to evaluate the new behavior.
Build successful! 🎉
## API Changes
unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any', access: 'private' } unknown top level export { type: 'any', access: 'private' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'identifier', name: 'Column' } unknown top level export { type: 'identifier', name: 'Column' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown type { type: 'link' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' } unknown top level export { type: 'any' }
@QzCurious apologies about the delay, but the team went through the behavioral changes in a testing session today. Overall, we liked the update in behavior that allows the user to now scroll through multiple calendars and finalize a range selection. However, we weren't sure about completely canceling the range selection upon clicking outside the calendar, namely because of a flow where the user might assume that their selection has been made and then they click on an accompanying TimeField to continue configuring their date as shown here:
https://github.com/adobe/react-spectrum/assets/9661127/6eff20f1-2a2c-4f8b-8993-14acc11db2e6
with the new changes that would get canceled:
https://github.com/adobe/react-spectrum/assets/9661127/0dddab9f-eaf9-40c5-ae70-c99df0ec5fcd
I understand there should have some effort for validating all possible scenarios. And im gald you found a "behavioral bug" for my change.
I'd love to ask if you team would have a plan to sort it all and implement it (because it actually involve some decisions instead of just a bug). Or I could still submit some progress, if any, for you to validate it.
By the way
For scroll through calendars, first recording is wrong. You should validating it on mobile interface (you can refer to the videos on https://github.com/adobe/react-spectrum/pull/5721#issue-2090932074).
From the team's discussion yesterday, we'd be happy to accept a change isolated to fixing the mobile scrolling range selection but it was hard to tell from a glance if that would be easy to pull out from the changes made in this PR. Would that be possible?
For scroll through calendars, first recording is wrong. You should validating it on mobile interface (you can refer to the videos on https://github.com/adobe/react-spectrum/pull/5721#issue-2090932074).
As for the first recording, that is intentionally showing the desktop behavioral change rather than the mobile behavior. We were concerned that the changes may be confusing to a user using the RSP DateRangePicker because the flow shown in the first video here used to allow a user to confirm a date range selection when they click into the neighboring TimeField but that wouldn't happen after these changes in the PR.
I see. Thanks for explaining it!
Let me know if you want keep these PR and issues open, or I will close this PR and related issues. And open an issue for "mobile scrolling range selection".
Close.
Summery
- This PR is doing two things
- Cancel ongoing selection when xxx, instead of select range on blur (see https://github.com/adobe/react-spectrum/pull/5721#issuecomment-1908310751)
- Allow (mobile) scroll through multiple calendars and finalize a range selection (see https://github.com/adobe/react-spectrum/pull/5721#issue-2090932074: Scrolling start on empty cell, td, title of calendar)
- This PR would break an expected behavior, a UI that combing a calendar and a TimeFile (see: https://github.com/adobe/react-spectrum/pull/5721#issuecomment-1965572524)
- It's preferable to only have "mobile scrolling range selection" fixed without breaking existing "select range on blur".