DateTimePicker: re-implement component with react-datepicker component #25428
Conversation
retrofox
added
[Feature] UI Components
|
Size Change: +102 kB (8%) 🔍 Total Size: 1.27 MB
ℹ️ View Unchanged
|
retrofox
force-pushed
the
update/post-schedule-switch-calendar-component
branch
from
5561271
to
36bf38c
Compare
| import DayPickerSingleDateController from 'react-dates/lib/components/DayPickerSingleDateController'; | ||
| import ReactDatePicker from 'react-datepicker'; | ||
| import { isSameDay, format, setMonth, getMonth, getYear, getDate } from 'date-fns'; | ||
| import { map, filter } from 'lodash'; |
There was a problem hiding this comment.
I'd recommend using the native JS [].map and [].filter over the Lodash versions when possible.
retrofox
changed the title
|
@retrofox I have a huge queue of tasks to go through 🙂 Will try go get to this as soon as possible. In the meantime, can you explain please what is the value added for users here? At first glance, this change seems prioritizing developers convenience over users experience.
Also, if possible, can you provide a list of the a11y improvements landed in |
I bet you have. Thanks in advance.
Not at all :-). From my POV, the goal for these improvements is to be able to:
We tried to do the same with the current dev libraries, I ran into many issues, here yes, from the development point of view.
These issues reported by you here:
Not 100% sure whether the last two ones are rightly handled, but if not, I'm guessing tackle them it isn't a big deal. The most important issues were about keyboard focus issues. Thanks for your feedback, Andrea. |
|
I'm definitely excited for this — the DateTime component has been rather difficult to work with as a third party. Hope to see this land and bring some needed improvements to the date picker :) Thanks @retrofox! |
|
@tellthemachines if I remember correctly you worked a lot on the current implementation. Any thoughts here? 🙂 |
|
Any chance that this switch will solve also #15348 ?
This has always been broken since WP 5.0 and still needs to be fixed... |
Happy to at least try to bring a solution. |
Let me cross the comment link and update the PR description, but in short, the answer is yes. It's easily doable. |
| icon={ 'arrow-left-alt' } | ||
| isSmall={ true } | ||
| onClick={ decreaseMonth } | ||
| /> |
There was a problem hiding this comment.
This button is empty so screen readers will only announce "button".
Te original react-datepicker button has both off-screen text and an aria-label.
There was a problem hiding this comment.
yes, agree. Keep in mind Andrea this PR is full of remaining issues. The primary purpose is to be able to know is switching the component will allow us to improve the whole implementation.
Anyway, I'm blad to keep update and polishing!
| icon={ 'arrow-right-alt' } | ||
| isSmall={ true } | ||
| onClick={ increaseMonth } | ||
| /> |
There was a problem hiding this comment.
Same as above: empty button
|
Tested a bit this PR and, from an accessibility perspective, there are a few problems to solve mainly related to keyboard navigation with screen readers and labelling. On a general level, I'm not sure react-datepicker has full support for internationalization and RTL languages; this is a technical part that should be evaluated separately. It's going to be a bit long so I'm going to use one comment for each issue 🙂 First: keyboard navigation with screen readers VoiceOver on mac works pretty differently from screen readers on Windows. The Windows ones (e.g NVDA, JAWS) use two main modes to navigate the content. Let's call these modes "browse mode" and "focus mode", even if the terminology changes depending on the screen reader. In browse mode, screen readers intercept most of the key presses for their internal usage. For example, the arrow keys are used to navigate the content. In this mode, key strokes are not passed to the browser so any script relying on keyboard events simply doesn't work. This is the main navigation mode used by users. Instead, focus mode is mainly used for form elements ot with some specific ARIA roles. Why this matters: Since selecting the day in the calendar month relies on keyboard events, in browse mode the day selection totally fails. See the animated GIF below that illustrates the behavior with Firefox and NVDA:
The reason why the calendar works in the current react-dates based implementation is that the DayPicker component uses an ARIA The Possible options: 1
I'm not sure there's a way to do that, though. 2 and then pass the custom container to the date picker: 3 For the records, I tested the option 2) and it fixes the problem. |
Thanks for your awesome feedback, Andrea. Very useful <3. I'm going to create a checklist in the PR description to organize the next actions to take. Please, feel free to add/edit. |
|
Labelling and localization / internationalization While the dates are "automatically" translated based on the locale, I see that react-datepicker uses some strings for its on user interface. I'm not sure these strings are translatable: they appear to be hardcoded. Also, at least one of these strings seems to be a bit pointless. For example:
The "Choose" prefix is hardcoded and seems it's not translatable: https://github.com/Hacker0x01/react-datepicker/blob/6d789febf11630ef4080084ad4744add92a89ab8/src/day.jsx#L240-L253 This prefix can be changed via the Other strings that seem not translatable:
I do see these can be changed via props but I'm not sure I understand how can they be translated? One more interesting question is: does react-datepicker support RTL languages. In the react-dates codebase I can see methods to support RTL but at a first glance I can't find anything related to RTL in react-datepicker. |
|
Labelling and Events The main purpose of this change is to add "events" on the calendar days. To start with, these events would be other posts published or scheduled for that day. Screenshot:
However, the tooltips alone are only a "visual thing". The tooltip documentation itself specifies that:
It is also worth reminding that the tooltip is rendered in the DOM very far from the element it is applied to so it can't be reached (nor it should be) via keyboard navigation. Also, tooltips can't contain interactive elements (e.g. links) because they would not be available to keyboard users. A tooltip is only a visual "hint" that should be accompanied with other accessible text to convey the same information to assistive technology users. Basically, when navigating through the days with a screen reader, there's no information whatsoever about the Events on that day that can be announced. So the point is: how do we communicate the Events information in a way that is available on the day buttons themselves? As noted in a previous comment, the buttons use an aria-label that overrides their content:
As far as I see, there's no way to change this aria-label. Only the "Choose" prefix can be changed. There are a few options that could be used to add the Events info within the day buttons but they would need the ability to change or remove the aria-label. This would require some investigation:
If the aria-label could be removed, we could just use This is probably the most challenging point for the Events implementation. |
|
Other issues worth mentioning (some of them are present also on the current implementation):
|
|
FYI: The increase in build size that the bot is reporting worried me at first, but I think this is just due to the fact that |
do you mean to the following? |
Yes, that's what I meant. The 101KB gzipped would have been a worryingly large increase for changes to a single component, on the order of 2.5 times the size of |
Thanks. Nice, we're going to create a new PR only for the component replacement. There, we're going pay attention to this. Do you mind if I add you as a reviewer? |
Not at all, happy to take a look! 🙂 |
It's nice to hear that :-)
That would be very nice. However, this PR suggests a huge refactoring that will take a lot of time, time that now I'm not sure whether I'll be able to provide. Thanks, folks. |
|
With #43005 merged, I believe that this PR can probably be closed. Of course, more a11y feedback on the current status of the component is welcome! cc @noisysocks |
Description
This PR exposes several issues in the current implementation of the
<PostSchdule />component, suggesting some solutions. Please take it as a look-into PR. The goal here is to be able to decide if these changes could work.Fixes #13713
Fixes #22387
Fixes #15348
Fixes #21820
react-dates vs react-datepicker
The biggest change here is replacing the react-dates component with the react-datepicker one. It's funny because we did the opposite almost two years ago.
Why switching again?
react-datepickergot rid of moment.js in favor of date-fns. There are many reasons why date-fns could be better than moment.js. Here there is a good comparison and analysis page. This is something that could be beneficial for the core-editor.react-datepickerwas replaced was some a11y issues. It has changed considerably, and now it has nice support for a11y. Not sure if it will enough.react-datepickerthan withreact-dates.For instance, we've used
<Button />component to redefine the calendar header nav buttons, and<Tooltip />to show date events for each calendar day when it's appropriated. Both of these components belong to@wordpress/components. The integration was almost flawless.a11y
As we said above, one of the tougher points of
react-datepickerwas about accessibility. Pleasantly, it has changed considerably lately on this component. It supports key navigation very well. And again, the integration with other components is very good too. Navigation through tabbing works as expected.Another important improvement added on this Pull Request is the screen reader support, allowing us fixing #15348.
It worths mentioning there are some weird issues (in master branch) when setting the date via the TimePicker component. Something about dealing with onBlur event. Here another hacky solution to attempt to handle it.
Highligh events
Highlighting site events in the Calendar is one of the goals behind all these changes. We tried with the
react-dateslibrary and it doesn't work in an acceptable way, IMO.This PR was the first attempt to tackle it. The bigger issue is how to propagate the events to the calendar to finally highlight them.
From the point of view if the
<PostSchedule />, events are site posts withpublishandfuturestatus. Naturally, these data are gotten asynchronously using thegetEntityRecords()selector. The problem is there isn't a good way (AFAIK) to refresh these data in the react-dates components.Also, this async flows happens when the month changes since we'd like to perform a request per month.
We've done some hacky solutions to deal with this, for instance, when hack to force the calendar to update on month or year change. In short, it's a lack of
onMonthChangecomponent event.Events Tooltip
As a nice extra improvement, we'd like to show events per day (no more than four) by each day. It's very useful to know if there already are scheduled posts on the site, for instance. It was kind of easily implemented using the
react-datepickercomponent.Other changes / improvements
Issue / Fixes
How has this been tested?
Editing a post / page / etc. Try to add other posts defining different dates in order to get some days highlighted. Also, for these days, do a mouse over the day in order to see the events tooltip.
Screenshots
Highlighted events
Events tooltip
📹 Live: Site events
It fetches events per month, every time that the month view changes. In this example,
📹 Live: a11y
All interactions are performed via the keyboard,
Types of changes
Checklist: