-
Couldn't load subscription status.
- Fork 1.3k
fix: Constrain day on blur #8385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the interest, it looks like this PR is breaking some tests. It would be nice to see those tests updated here so that we know what changed logically.
I would also expect to see new tests for this to prevent regressions in the future, would you mind adding some? Or, if you think it's too early, a description of how to test the changes as you view them in storybook.
Both of these are good to include so that we can follow your thinking and can discuss any assumptions.
|
Thanks @snowystinger for taking the time to check this. I’ll finish the work and mention you. |
…ctrum into constrain-date
…ctrum into constrain-date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the long delay in feedback and thank you once again for picking this up! The behavior and code look pretty good to me, just noted one thing that when fixed locally seems to fix the behavior to match #3256 (comment). I'll create a branch so the team can test off the build but lemme know if the proposed change makes sense
| constructor(calendar: Calendar, era: string, year: number, month: number, day: number, constrainDay?: boolean); | ||
| constructor(...args: any[]) { | ||
| let [calendar, era, year, month, day] = shiftArgs(args); | ||
| let [calendar, era, year, month, day, constrainDay] = shiftArgs(args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed when testing locally that typing in 2/30/2 (aka Feb 30 2xxx) into a datefield would constrain the 30 into 28. Looks like shiftArgs doesn't handle constrainDay so the provided constraintDay from https://github.com/boutahlilsoufiane/react-spectrum/blob/47d9cab7f5e62e5fb894543c17633a69cbafd498/packages/%40internationalized/date/src/manipulation.ts#L154 isn't coming through. However, shiftArgs might also be handling timezones/etc so either we need to tweak shiftArgs and clamp down on its type definition so we know what args may follow after day OR perhaps you can just grab constraintDay from the last arg in args here and just double check that it is a boolean.
Making that change locally seemed to fix the behavior for me, but a couple of tests failed since now we aren't truly constraining in the set call by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the expected behavior is to constrain day on blur, I think I changed something by mistake. I attempted to fix it in the last commit, but the tests are still failing. I'll investigate further to resolve this.
|
@boutahlilsoufiane did you happen to try the |
|
Hi @LFDanLu, thank you for reviewing this. The implementation follows the route, with one adaptation: I used |
…/react-spectrum into constrain-date
…/react-spectrum into constrain-date
|
@boutahlilsoufiane Thanks for the update! The team tested the implementation today and had some feedback, specifically around some specific cases that make So instead of modifying |
Closes #5965 #3256
Implements the behavior described here: #3256 (comment)
This adds a
constrainDayoption to many of the date manipulation/CalendarDate functions so the user can update a date without automatically constraining the day value. Note that this changes the behavior of many of the setters as well as the default of the DateField to NOT constrain by default, to be discussed if we want to change this for the setters.✅ Pull Request Checklist: