Skip to content

Conversation

QzCurious
Copy link
Contributor

@QzCurious QzCurious commented Jan 19, 2024

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:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • 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
Screen.Recording.2024-01-20.at.12.23.54.AM.mov
Screen.Recording.2024-01-20.at.12.25.01.AM.mov
Scrolling start on empty cell, td, title of calendar (or anywhere outside the calendar) should not affect selection
Screen.Recording.2024-01-20.at.12.41.27.AM.mov
Screen.Recording.2024-01-20.at.12.42.41.AM.mov
Drag onto empty cell, td, title of calendar or anywhere outside
Screen.Recording.2024-01-20.at.1.03.47.AM.mov
Screen.Recording.2024-01-20.at.1.02.52.AM.mov
Remain the same Original My Proposal
Scrolling start on a CalendarCell
Screen.Recording.2024-01-20.at.12.46.00.AM.mov
Screen.Recording.2024-01-20.at.12.44.43.AM.mov
Change viewport by navigation buttons
Screen.Recording.2024-01-20.at.12.51.30.AM.mov
Screen.Recording.2024-01-20.at.12.50.52.AM.mov
Select range by dragging
Screen.Recording.2024-01-20.at.12.54.00.AM.mov
Screen.Recording.2024-01-20.at.12.54.50.AM.mov

🧢 Your Project:

https://github.com/QzCurious/react-spectrum

@QzCurious
Copy link
Contributor Author

I don't know why but it seems like there were a few test case failed on commits before my.

Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and the detailed testing instructions!

First, you'll need to sign the Adobe CLA to get that build step to pass.

And it looks like the tests that are failing are correctly failing due to the updated behavior (i.e. clicking outside calendar commits selection). We'll need to update those to match the new expected behavior.

If any tests were failing before your changes, they were probably related to a node version mismatch and can be ignored (they'll pass in CI).

@QzCurious QzCurious closed this Jan 20, 2024
@QzCurious QzCurious reopened this Jan 20, 2024
@QzCurious QzCurious closed this Jan 20, 2024
@QzCurious QzCurious reopened this Jan 20, 2024
@QzCurious
Copy link
Contributor Author

QzCurious commented Jan 20, 2024

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 ee51290).
image

I'm not sure these test cases were "correctly failing" or caused by my environment.
image

@LFDanLu
Copy link
Member

LFDanLu commented Jan 24, 2024

@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

@QzCurious
Copy link
Contributor Author

It works on v18.12.1 rather.

image


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

@QzCurious
Copy link
Contributor Author

QzCurious commented Jan 24, 2024

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

Screenshot 2024-01-24 at 11 09 55 PM
Screenshot 2024-01-24 at 11 10 05 PM
Screenshot 2024-01-24 at 11 10 17 PM
Screenshot 2024-01-24 at 11 10 30 PM

@QzCurious QzCurious requested a review from reidbarber January 30, 2024 23:48
@reidbarber
Copy link
Member

GET_BUILD

@rspbot
Copy link

rspbot commented Jan 31, 2024

@QzCurious
Copy link
Contributor Author

Hi, just wanted to know how it goes. Let me know if there's anything I can help with.

@LFDanLu
Copy link
Member

LFDanLu commented Feb 20, 2024

@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.

@LFDanLu
Copy link
Member

LFDanLu commented Feb 23, 2024

GET_BUILD

@rspbot
Copy link

rspbot commented Feb 23, 2024

@rspbot
Copy link

rspbot commented Feb 23, 2024

## 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' }

@LFDanLu
Copy link
Member

LFDanLu commented Feb 27, 2024

@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:

Screen.Recording.2024-02-26.at.4.23.58.PM.mov

with the new changes that would get canceled:

Screen.Recording.2024-02-26.at.4.24.54.PM.mov

@QzCurious
Copy link
Contributor Author

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 #5721 (comment)).

@LFDanLu
Copy link
Member

LFDanLu commented Feb 27, 2024

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 #5721 (comment)).

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.

@QzCurious
Copy link
Contributor Author

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".

@LFDanLu
Copy link
Member

LFDanLu commented Feb 28, 2024

Feel free to close them and open a new issue!

@QzCurious
Copy link
Contributor Author

Close.

Summery

  1. This PR is doing two things
  2. This PR would break an expected behavior, a UI that combing a calendar and a TimeFile (see: fix useRangeCalendar behavior while interacting outside #5721 (comment))
  3. It's preferable to only have "mobile scrolling range selection" fixed without breaking existing "select range on blur".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants