DateTimePicker: Replace react-dates and moment with useLilius and date-fns#43005
Conversation
talldan
left a comment
There was a problem hiding this comment.
Looks great, it's so nice to be getting rid of that keepFocusInside hack. 🎉
There was a problem hiding this comment.
I guess the alternative is to set focus in the onKeyDown event handler directly. Is that something you considered? It'd mean there's no need for isFocusAllowed.
I guess you might have to use a data attribute or something to find the appropriate Day element in the DOM that way, and the event handler would probably need to be bound to the parent month.
It works nicely as it is, so no need to change it, just curious.
Should isFocusAllowed be in the deps?
There was a problem hiding this comment.
I'm glad you're here! This keyboard stuff took me a day to get right and the whole time I was thinking of you and your struggles with RovingTabIndex 😂
I guess the alternative is to set focus in the
onKeyDownevent handler directly. Is that something you considered? It'd mean there's no need forisFocusAllowed.
Yeah, I did think about it, but it makes it difficult to implement the nice behaviour where if you arrow past the beginning/end of the grid it automatically switches to the previous/next month. You have to wait until the new month renders before calling focus() since otherwise the element won't exist in the DOM yet. You get this for free when using this state approach since React will call useEffect() after rendering the new state.
Should isFocusAllowed be in the deps?
If we did this then we'd be calling focus() on an element that already has focus. I suppose it doesn't actually matter... idk, I like that the intention here is explicit which is that we only want to focus the element when isFocusable changes to true 😀
There was a problem hiding this comment.
Might be worth adding a comment to the code to explain why it's missing from the deps. It's something a well meaning dev would change.
There was a problem hiding this comment.
OK, I tried to explain it all using some comments, although it feels like I just wrote the word "focus" a dozen times in a row 😅
There was a problem hiding this comment.
getDayLabel looks like a candidate for useMemo.
There was a problem hiding this comment.
Agreed with you but then I ended up having to remove the push stuff from getDayLabel because it didn't really work in RTL languages. This means getDayLabel is now a simple if and therefore I think not necessary to memoize.
(Could also React.memo the whole of Day if we wanted to. Idk. Probably it's premature optimisation...)
…e-fns Updates DateTimePicker, TimePicker and DatePicker to use date-fns and useLilius instead of react-dates and moment. Both libraries result in less minified bytes sent to the client. useLilius also has other advantages: less component remounting, can use dateI18n to localise labels, can use @wordpress/components components, can use Emotion instead of SCSS.
d30f194 to
1951ead
Compare
|
Size Change: -38.7 kB (-3%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
| role="application" | ||
| aria-label={ __( 'Calendar' ) } |
There was a problem hiding this comment.
I copied this from the previous implementation but I'm not sure if role=application is the right thing to do. Maybe someone who knows what they're doing can answer cc. @talldan @tellthemachines
|
I've been testing this using keyboard and mouse interactions in LTR and RTL langs and it's working great!
🎉 |
talldan
left a comment
There was a problem hiding this comment.
LGTM, thanks for tackling this difficult task!
ciampo
left a comment
There was a problem hiding this comment.
Hey @noisysocks thank you for working on this!
Even though this PR has already been merged, I went ahead and gave it a good look.
Apart from my inline comments, here's some additional feedback:
-
we should add
box-sizingreset styles to the root element of each component (DatePicker,TimePicker,DateTimePicker) -
When the calendar is focused, pressing arrow keys scrolls the page (we should preventDefault or stopPropagation)
-
The "calendar help" text was not updated to remove support for page up / page down / home / end keys (unless we didn't mean to remove support for those keys)
-
The font size "day of the week" text (i.e "Wed", "Thu") should be set in a way that doesn't change when the wordpress Global Styles are added or removed (you can use the switch in Storybook to toggle them on/off)
-
Could we look into replacing
userEvent.setup( { delay: null } )withconst user = userEvent.setup( { advanceTimers: jest.advanceTimersByTime } )?
A couple of additional thoughts:
- The component's chosen timezone seem to be tightly coupled with the WordPress editor's settings — we should work on a way to decouple this, and maybe have the desired timezone offset as a prop?
- It would be great if the @wordpress/components
package didn't import any date library at all (i.e.date-fns), and instead used@wordpress/date` as its date utility library - Of course, we should look into migrating
@wordpress/dateaway frommomenttoo, but that's a whole different story
(cc'ing @sgomes since he was involved in previous conversations where we evaluated Temporal).
|
|
||
| await user.click( | ||
| screen.getByRole( 'button', { name: 'Friday, May 20, 2022' } ) | ||
| screen.getByRole( 'button', { name: 'May 20, 2022' } ) |
There was a problem hiding this comment.
Is there a reason why we changed the button's accessible name (removing the week-day name) in this refactor, instead of making the new implementation match the old one?
There was a problem hiding this comment.
Yes! Using the format string in wp.date.__experimentalGetSettings().formats.date instead of a hardcoded format string ensures that the ARIA label respects the site's Date Format setting:
It also ensures that the format string is internationalised because the format string obtained from wp-date has been passed through __:
This is important as not all languages use {Day}, {Month} {Day}, {Year}.
Worth noting that prior to this PR these labels weren't working anyway. The labels contained format string tokens:
There was a problem hiding this comment.
Sounds good, thank you for the context!
"Are you free on Thursday F, j?"
| day: format( date, 'dd' ), | ||
| month: format( date, 'MM' ), | ||
| year: format( date, 'yyyy' ), | ||
| minutes: format( date, 'mm' ), | ||
| hours: format( date, is12Hour ? 'hh' : 'HH' ), | ||
| am: format( date, 'a' ), |
There was a problem hiding this comment.
Could these also be moved to the constants file?
There was a problem hiding this comment.
The format strings? I think it's nice having them near to where they're used as the reader has to jump around less.
There was a problem hiding this comment.
I see, although I thought that would be more coherent with having the TIMEZONELESS_FORMAT as a variable in a separate file.
There was a problem hiding this comment.
The difference there is that TIMEZONELESS_FORMAT is used by two components (TimePicker and DatePicker) so makes sense to have it in a separate file where both can get at it.
| }; | ||
|
|
||
| const momentDate = getMomentDate( currentDate ); | ||
| }, [ isFocusable ] ); |
| const { formats } = __experimentalGetSettings(); | ||
| const localizedDate = dateI18n( | ||
| formats.date, | ||
| date, | ||
| -date.getTimezoneOffset() | ||
| ); |
There was a problem hiding this comment.
Previously we were not relying on @wordpress/date:
- could we use
date-fnsfor formatting the date instead ? - alternatively, should we aim at using
@wordpress/datemore instead ofdate-fns?
Ultimately, I'm trying to understand if we get to "one source" of date utils, instead of using both third party libraries AND internal libraries.
There was a problem hiding this comment.
We could use format from date-fns but we would need to set up internationalisation which is a little tricky to do in a way that doesn't increase the size of the build. We'd have to pass the locale to the component which I'm not a huge fan of because it makes it difficult to switch DatePicker away from date-fns in the future.
https://date-fns.org/v2.16.1/docs/I18n
dateI8n from @wordpress/date already handles localisation and is configured properly by WordPress which is why I'm using it here.
My view on @wordpress/date is that it is complimentary to a date util library (e.g. date-fns) and should only provide:
- A store for the WordPress site's date settings including format preference and timezone.
- A function similar to
wp_datein PHP.
Everything else should be deprecated from @wordpress/date in favour of date-fns. Right now I believe that's just the isInTheFuture function.
There was a problem hiding this comment.
Gotcha. Not something that needs to be addressed urgently, but I'll see if I can open an issue on next steps for @wordpress/date (which should include what we discussed in this comment too)
|
Thanks so much @ciampo! I've opened #43495 to address your notes.
We are relying on the date format that you get from
I wrote my thoughts on this in #43005 (comment). In short I think both packages are complimentary.
Yes! |
What?
Follows #41385.
Updates
DatePickerto useuseLiliusinstead ofreact-datesto render its calendar and removesmomentas a dependency from@wordpress/componentsin favour ofdate-fns.https://github.com/its-danny/use-lilius
https://github.com/date-fns/date-fns
Why?
Problems with
react-datesmomentwhich is quite large and no longer actively developed.useEffectto set anaria-labelon the day button.keywhen the date is changed. (This makes the whole thing feel really slow.)@wordpress/componentscomponents. Many of the UI elements can't be swapped out with@wordpress/componentscomponents. This results in a less consistent UI and more CSS shipped to the browser.Considering
react-calendarI looked at using
react-calendarin #41385. This is a smaller alternative toreact-datesthat usesdate-fns.Pros:
date-fns, allowing us to removemomentas a dependency.Cons:
@wordpress/componentscomponents. Many of the UI elements can't be swapped out with@wordpress/componentscomponents. This results in a less consistent UI and more CSS shipped to the browser.Considering
useLiliusI then looked at
useLiliusper a suggestion by @stokesman. This is a "headless" library that usesdate-fns. It offers a single hook that can be used to display a calendar and not any actual UI components.Pros:
date-fns, allowing us to removemomentas a dependency.@wordpress/componentscomponents and Emotion for all of the UI.Cons:
Considering
TemporalThe
TemporalAPI is now stage 3, so I considered a completely bespoke implementation usingTemporal.Pros:
Temporal.PlainDateis perfectly suited for implementing a calendar as it does not contain time or timezone information, both of which are not relevant in the context of choosing a date from a calendar.Temporalwill assumedly be a native browser API in due course.Cons:
Temporalin production.Temporal. What's more,Temporalis much more difficult to tree shake thandate-fnsbecause all of the functions are implemented as methods on e.g.Temporal.PlainDate.prototype. This means the polyfill we ship is quite large.Conclusion:
useLilius+date-fnsI settled on using
useLiliusand rolling our own implementation of keyboard navigation. This approach offers the most upfront benefit and is relatively straightforward to implement.This approach doesn't preclude a later switch to
Temporal. When theTemporalproposal is more developed and implemented by at least one widely used browser (e.g. Chrome), it will be less likely that a user needs to download a heavy polyfill. We should then be able to easily implement our own version ofuseLiliusthat usesTemporal.PlainDate. If you look at the source ofuseLiliusyou'll see that it's quite small.How?
@wordpress/components: Removemomentandreact-datesdependencies. Adddate-fnsanduse-lilius.DateTimePicker: UseVStackso that we can remove all CSS.TimePicker: Swapmomentfunctions fordate-fnsfunctions.DatePickeruseLiliusinstead ofreact-dates.@wordpress/componentsand Emotion instead of CSS.dateI18nso that dates are correctly translated.wp.date.getSettings()so that dates appear in the selected format.Testing Instructions
Screenshots or screencast
Before:
before.mp4
After:
after.mp4
I don't know if it comes across on video but it feels much snappier to use because we're no longer remounting the component 🙂