Skip to content

Conversation

@boutahlilsoufiane
Copy link
Contributor

@boutahlilsoufiane boutahlilsoufiane commented Jun 11, 2025

Closes #5965 #3256

Implements the behavior described here: #3256 (comment)
This adds a constrainDay option 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:

  • 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

Copy link
Member

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

@boutahlilsoufiane
Copy link
Contributor Author

Thanks @snowystinger for taking the time to check this. I’ll finish the work and mention you.

Copy link
Member

@LFDanLu LFDanLu left a 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);
Copy link
Member

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

Copy link
Contributor Author

@boutahlilsoufiane boutahlilsoufiane Oct 24, 2025

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.

@LFDanLu LFDanLu mentioned this pull request Oct 22, 2025
5 tasks
@LFDanLu
Copy link
Member

LFDanLu commented Oct 22, 2025

@boutahlilsoufiane did you happen to try the IncompleteDate route mentioned #3256 (comment) by the way? Just curious if you ran into any issues with that.

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Oct 24, 2025

Hi @LFDanLu, thank you for reviewing this. The implementation follows the route, with one adaptation: I used placeholderDate instead of IncompleteDate since the specifications for IncompleteDate weren't fully detailed. Happy to implement it differently if you have specific requirements.

@LFDanLu
Copy link
Member

LFDanLu commented Oct 27, 2025

@boutahlilsoufiane Thanks for the update! The team tested the implementation today and had some feedback, specifically around some specific cases that make IncompleteDate more desirable here. We noticed that only the day is allowed to be unconstrained but there are various cases where fields like the month or year may need to be unconstrained (e.g. lunisolar calendars like Hebrew will sometimes have a 13th month, Japanese calendars have eras that may only span a set of years before resetting aka Reiwa 7 = 2025, some hours won't exist due to daylight saving skip ahead). Because of this it would be ideal if there was something like IncompleteDate that would accept any arbitrary date value regardless of its validity/completeness as described in #3256 (comment). Any thoughts on these cases above?

So instead of modifying @internationalized/date to allow the values to represent invalid dates, can we create a new IncompleteDate class within @react-stately/datepicker to represent these values? Lots of functions in @internationalized/date expect the value to be valid and might break if they are not. We think constraining this to the UI-level code rather than the general purpose date library would be ideal.

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

Projects

Status: 🩺 To Triage

Development

Successfully merging this pull request may close these issues.

Datepicker field - Inconsistency in accepting leap day input via keyboard

3 participants