[WNMGDS-2757] Upgrade react-day-picker library
#3064
Conversation
To reproduce the error, go to the day picker story and click the calendar button. It will throw an error about `.current` and coming from a jsx function I wonder if the change [between react-day-picker v8.8.2 and v8.9.0](gpbl/react-day-picker@v8.8.2...v8.9.0) causing problems is the removal of the React imports from the modules. I know we've had issues with that in the past. Right now if I upgrade past v8.8.2, Storybook will work with React but not with Preact. I think in order for us to be able to use libraries that use the new JSX transform, we have to use the new JSX transform, but we can't use the new JSX transform until we can safely drop support for React 16. I've tried changing the TS config setting to ```json "jsx": "react-jsx", ``` but it doesn't seem to fix the problem.
…t projects It only works if Preact is in React-compatability mode. It works in examples/preact-react-app but not examples/preact-app.
…les support While this doesn't work in this commit, the whole goal of this effort is to make it work. The problem is figuring out how to get their built-in JSX transform functions to play nice with the way _we_ handle JSX, React, and Preact.
These changes were made after discussion with both Accessibility and Design and have been approved by UX leads
pwolfert
added
Type: Fixed
There was a problem hiding this comment.
The library may have an a11y bug. Or it's how we've implemented it. Or it doesn't matter! 😵
So I've found the calendar picker does accurately prevent user interaction (users can't use keyboards or pointers to select out of range dates, and that's good!) but if a user types a date that's out of range, the picker applies aria-selected="true" to a disabled date! Is this a bug? I don't know! Maybe disabled has higher specificity and aria-selected doesn't matter?
Screen.Recording.2024-05-01.at.10.33.48.AM.mov
| } | ||
|
|
||
| .rdp-button:not([aria-disabled='true']) { | ||
| .rdp-button:not(:disabled) { | ||
| cursor: pointer; |
There was a problem hiding this comment.
This is an odd rule - our buttons have this set in our reset file so this must be undoing another style this library has set?
There was a problem hiding this comment.
Could be redundant; I'll check
There was a problem hiding this comment.
Yeah, they have a button reset rule of their own. That could be something to revisit later—whether we want to try to optimize their CSS. At the beginning, our strategy was to try to keep their original CSS mostly intact and add overrides to it so it would be easy to upgrade the library as time went on. With that strategy, though, we do include a lot more rules than we need.
| @@ -67,12 +67,7 @@ | |||
| border: 2px solid transparent; | |||
There was a problem hiding this comment.
OK, see my comment below on the cursor pointer, but this rdp-button_reset class is doing a lot of redundant things too. I think we only need the background: none bit but this could use a deeper dive to ensure that doesn't introduce regressions.
There was a problem hiding this comment.
See comment above. I think this is out of scope for this work right now, but we should follow up on it.
|
|
||
| @media (forced-colors: active) { | ||
| background-color: GrayText; | ||
| cursor: not-allowed; |
There was a problem hiding this comment.
Because this has point-events:none I don't think cursor: not-allowed will work
There was a problem hiding this comment.
Oh, that's interesting! Good catch. When I go back and look at their PR that switched back from aria-disabled to disabled, it looks like they removed the pointer-events: none because it's not needed when the browser already knows the button is disabled.
…e pointer events And this keeps us from being able to apply the `not-allowed` cursor style
|
@zarahzachz, about the odd behavior you found with out-of-range dates:
I don't think we should wade into the territory of implementing validation on this component, and I think that would fall in that category. The app team is responsible for supplying those date ranges, so they should be validating and showing an error message. Right now it does appear that the disabled style has higher specificity, so at least we aren't showing that they've selected an invalid date in the calendar. I just tested with VoiceOver, and the fact that we don't move focus to the selected button when the calendar is opened makes me believe a screen reader user will never hear that selected state in normal usage. |
* This is the minimum version that causes the issue in Preact Storybook To reproduce the error, go to the day picker story and click the calendar button. It will throw an error about `.current` and coming from a jsx function I wonder if the change [between react-day-picker v8.8.2 and v8.9.0](gpbl/react-day-picker@v8.8.2...v8.9.0) causing problems is the removal of the React imports from the modules. I know we've had issues with that in the past. Right now if I upgrade past v8.8.2, Storybook will work with React but not with Preact. I think in order for us to be able to use libraries that use the new JSX transform, we have to use the new JSX transform, but we can't use the new JSX transform until we can safely drop support for React 16. I've tried changing the TS config setting to ```json "jsx": "react-jsx", ``` but it doesn't seem to fix the problem. * Discovered that our SingleInputDateField doesn't work in native Preact projects It only works if Preact is in React-compatability mode. It works in examples/preact-react-app but not examples/preact-app. * Upgrade to the version of RDP that gives us date-fns 3.0 with ES Modules support While this doesn't work in this commit, the whole goal of this effort is to make it work. The problem is figuring out how to get their built-in JSX transform functions to play nice with the way _we_ handle JSX, React, and Preact. * Upgrade date-fns too. It all works in the Astro example * Upgrade to fixed version * Fix `preact-app` example * Update CSS because of `aria-disabled` to `disabled` change upstream * Fix selectors * Address color-contrast issues with design changes These changes were made after discussion with both Accessibility and Design and have been approved by UX leads * Fix strike-through in forced-colors mode * Update VRT refs * Add it to the astro example too * Remove from examples where it isn't needed now that we've tested it * Update VRT refs of astro example where we added the date field * Now that these buttons are actually disabled, we don't need to disable pointer events And this keeps us from being able to apply the `not-allowed` cursor style
Summary
WNMGDS-2757
See background in the ticket and in #2920. We're upgrading the
react-day-pickerlibrary primarily because the old version did not support ES Modules, which causes problems with modern build tools.Before we could do this upgrade, we had to contribute a fix upstream for Preact support, and that resulted in this release, which is what we're upgrading to.
Note that this upgrade will drop our uncompressed React bundle size from
407KBto387KB.Changes:
react-day-pickerdependency from8.0.5to8.10.1date-fnsdependency from2.28.0to3.0.6How to test
Checklist
[WNMGDS-####] Titleor [NO-TICKET] if this is unticketed work.Type(only one) label for this PR, if it is a breaking change, label should only beType: BreakingImpacts, multiple can be selected.If this is a change to code:
yarn test:unit:update) and browser-test snapshots (yarn test:browser:update)