Design updates to the Publish popover (DateTimePicker)
#41097
Conversation
noisysocks
added
[Type] Enhancement
|
Size Change: +352 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I've still got some tasks remaining before we can merge this (see description) but I'm happy with the broad strokes and think this is ready for a first round of review. |
|
Thanks for the ping @noisysocks, I think I'll run out of time today to take a closer look, but I'm looking forward to getting stuck into review 🤓 At first glance, the design's looking really nice in the editor 👍
One thing I noticed is that if you click on the AM / PM buttons it closes the popover, unlike the other buttons in the popover. Not sure if focus is being moved somewhere? |
👍 I made it so that the popover remains open. The alternative is that we auto-close it after a date is selected but I don't like that because then it makes it hard to select a date and a time. Also we have an X now. |
|
This is looking great! I just have just one comment: When you have scheduled another post we show a small blue dot under the day number. But if you hover over the day or select it, the dot disappears. I propose changing the color of the dot so it's always visible. That would mean white for when the day is selected or selected and hovered. Screen.Recording.2022-05-18.at.12.31.26.movThen I noticed a weird reload the first time you select a day from a different month. I guess this is not related to this PR, but I just wanted to mention it anyway in case it's helpful. Screen.Recording.2022-05-18.at.12.24.05.mov |
| ? Math.min( max, Math.max( min, value ) ) | ||
| : roundClamp( value, min, max, stepOverride ?? baseStep ); | ||
| return number.toString().padStart( pad, '0' ); |
There was a problem hiding this comment.
The 0 padding looks like a useful feature!
From a quick glance, it looks like this changes the implicit return type for constrainValue from a number to a string. Would it be worth splitting out this change (and the change to SelectControl) into a separate PR(s)? Since NumberControl is fairly widely used, and can receive a state reducer, it might be worth us pinging a few folks on the change in isolation to get a few more eyes on the potential implications (I've been tripped up trying to make changes to NumberControl before 😄).
There was a problem hiding this comment.
Now that I think of it, the conversion to string might not be such a problem (and could be desired). I see from @ciampo's earlier NumberControl PR to refactor to TypeScript (#38753) it proposes introducing an ensureString function that gets used in the reducer, so the change here might be useful for that, too.
There was a problem hiding this comment.
Sure, I can split this out!
Changing the return type to string is intentional. If you look at the typings for the reducer state in SelectControl you'll see that value is supposed to be a string.
There was a problem hiding this comment.
Great, thanks for confirming!
There was a problem hiding this comment.
I took NumberControl for a spin and played around with the new pad property, and found a few instances where it behaved unexpectedly (for example, the 0 padding is not added when typing a value and blurring the input field).
number-control-padding.mp4
Considering that InputControl (and consequently NumberControl) is undergoing a few changes / fixes, and that NumberControl isn't typed yet, my recommendation for now is to add this feature directly to TimePicker if possible (and consider porting it to NumberControl once those fixes/improvements are applied).
There was a problem hiding this comment.
my recommendation for now is to add this feature directly to TimePicker if possible (and consider porting it to
NumberControlonce those fixes/improvements are applied).
That sounds like a good idea to me, too.
noisysocks
force-pushed
the
update/date-time-picker-design
branch
from
3f24700
to
6731934
Compare
Good thinking! I've done this.
Let's look into this separately. It happens because of react-dates/react-dates#1320 which is fixed but not yet released. |
|
OK, happy with this now 🙂 I fixed unit tests, tweaked some of the margins, fixed some issues with RTL languages, and tested everything in multiple browsers. I'll split out some of the |
There was a problem hiding this comment.
Thank you @noisysocks for working on this, it's looking great!
I've left most of my feedback via inline comments, but I'll add a few more points here:
Re-organises DateTimePicker folder structure to be more consistent with newer components.
Each sub-component in date-time now has its own folder.
Thank you for improving the folder structure! Ideally, to follow the latest guidelines, there would be more changes required (e.g. splitting each component into a hook and component files, adding READMES for each subcomponent...), but those changes can definitely wait and be applied at a later point as a follow-up.
Also, we normally keep only one stories folder per component family, instead of having a stories folder in each subcomponent's folder — would you mind applying those changes?
Removes Reset button and Help functionality from DateTimePicker.
The new design does not have this.
This maybe is a breaking change. We can phase this in using a prop if we feel that it's too much of a change to impose on third parties.
For the sake of these changes, I would keep it around. We can look into adding a soft deprecation warning about it, and remove it later down the road (e.g. after WordPress 6.2 gets released)
I'll split out some of the @wordpress/components stuff into a separate PR for ease of review tomorrow, but don't let that stop you from giving this a whirl!
That would be awesome — I can see a few PRs can could be splintered off:
- updating
@types/react-dates's version - updating
moment's version - changes to
NumberControl(if any) - changes to
SelectControl - changes to
DateTimePicker - remaining updated in the
block-editorandedit-sitepackages
But again — we can keep these changes together in the same PR for now, for the sake of iterating quicker and have a first few rounds of reviews
packages/block-editor/src/components/publish-date-time-picker/style.scss
Outdated
Show resolved
Hide resolved
|
Thanks for the reviews! I've addressed all the feedback. I'll split the PR up on Monday. |
There was a problem hiding this comment.
Thank you! I left a few more replies to your comments
Thanks for the reviews! I've addressed all the feedback. I'll split the PR up on Monday.
Awesome! I haven't had the time to look at all of your latest changes in details (like, for example, the opt-out flag for the reset button), but i'll make sure I do in each split PR
There was a problem hiding this comment.
I recommend setting GitHub to only show the second commit in this PR with Hide whitespace checked so that the diff is less noisy.
Thank you for committing your changes in a reviewer-friendly way!
Some of the e2e failures are legitimate, probably because some classnames have been removed, I'll take a look at them tomorrow 🙂
Good point. In the meantime I had another look at the changes and left a few more inline comments around.
| @@ -1,9 +1,6 @@ | |||
| /** | |||
| * External dependencies | |||
| */ | |||
| // Needed to initialise the default datepicker styles. | |||
| // See: https://github.com/airbnb/react-dates#initialize | |||
| import 'react-dates/initialize'; | |||
There was a problem hiding this comment.
For ease of review, noting that this line has been moved to packages/components/src/date-time/date/index.tsx
noisysocks
force-pushed
the
update/date-time-picker-design
branch
from
b87608b
to
cdcc28c
Compare
|
OK, tests fixed! I want to keep the e2e changes minimal here so I did not switch away from using CSS selectors, even though that's not really ideal. It would be good to update |
| since: '13.4', | ||
| version: '14.6', // Six months of plugin releases. | ||
| hint: | ||
| 'Set the `__nextRemoveHelpButton` prop to `true` to remove this warning and opt in to the new behaviour, which will become the default in a future version.', | ||
| } ); | ||
| } | ||
| if ( ! __nextRemoveResetButton ) { | ||
| deprecated( 'Reset button in wp.components.DateTimePicker', { | ||
| since: '13.4', | ||
| version: '14.6', // Six months of plugin releases. |
There was a problem hiding this comment.
Interesting. I believe that I always used the WordPress version (and not the Gutenberg plugin version) when working on components. Would be interesting to see which approach is the expected one!
There was a problem hiding this comment.
I've always seen and used the Gutenberg version... it's an interesting question because the code ends up both in the plugin and Core... hmmm! 🤔
There was a problem hiding this comment.
At least the version numbers in Gutenberg are way ahead of core, so it's easy to tell them apart 😁
| const year = await getInputValue( '[aria-label="Year"]' ); | ||
| const month = await getInputValue( '[aria-label="Month"]' ); | ||
| const monthLabel = await getSelectedOptionLabel( '[aria-label="Month"]' ); | ||
| const day = await getInputValue( '[aria-label="Day"]' ); | ||
| const hours = await getInputValue( '[aria-label="Hours"]' ); | ||
| const minutes = await getInputValue( '[aria-label="Minutes"]' ); |
There was a problem hiding this comment.
Just wondering if it wouldn't be a good thing to add these aria-labels in our code (unless those inputs are already labelled in an accessible way)
There was a problem hiding this comment.
The inputs are labelled, but it's done using <label for=""> which you can't target using a CSS selector. I thought about using puppeteer-testing-library here so that we can do this properly but I think it's too great of a change for this PR.
packages/block-editor/src/components/publish-date-time-picker/README.md
Outdated
Show resolved
Hide resolved
- Updates the design of DateTimePicker, DatePicker and TimePicker. - Converts CSS in DateTimePicker, DatePicker and TimePicker to Emotion where it makes sense to do so. - Allows removal of Reset button and Help functionality from DateTimePicker. - Adds PublishDateTimePicker to block-editor. This exposes a specialised DateTimePicker that is good for choosing a post publish date.
…veResetButton are not given
noisysocks
force-pushed
the
update/date-time-picker-design
branch
from
c79f127
to
d93ff2b
Compare
There was a problem hiding this comment.
Great work @noisysocks, this is feeling very polished to me while testing in the editor and Storybook, and the code change looks solid. Nice work moving the zero padding to a state reducer, too, that looks like a good and logical way to handle the padding behaviour for now.
LGTM! 🎉
There was a problem hiding this comment.
Nice work wrangling all of this @noisysocks 🙇
✅ Storybook examples are functioning well
✅ Works great in the editor
✅ All unit tests and e2es are passing locally
LGTM! 🚢
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { getMomentDate } from './utils'; | ||
| import type { DatePickerDayProps, DatePickerProps } from './types'; | ||
| import type { DatePickerDayProps, DatePickerProps } from '../types'; | ||
| import { Day, NavPrevButton, NavNextButton } from './styles'; | ||
|
|
||
| const TIMEZONELESS_FORMAT = 'YYYY-MM-DDTHH:mm:ss'; | ||
| const ARIAL_LABEL_TIME_FORMAT = 'dddd, LL'; |
There was a problem hiding this comment.
Not necessary for this PR (it's in trunk) I noticed that the aria-label on the day component outputs symbols.
Not sure if the problem is in moment(). It's something to do with the locale aware format LL
This works
const ARIAL_LABEL_TIME_FORMAT = 'dddd, MMMM DD, YYYY'; // Monday, May 09, 2022But it's not locale aware I presume so it might need some further 👀
There was a problem hiding this comment.
Oh interesting, good catch. We probably should use our own dateFormat function that's in @wordpress/date here:
I'll turn your comment into an issue.
|
Thanks everyone, big love ❤️ |
* Re-organise date-time dir to have component subdirs * Update design of DateTimePicker and convert to Emotion - Updates the design of DateTimePicker, DatePicker and TimePicker. - Converts CSS in DateTimePicker, DatePicker and TimePicker to Emotion where it makes sense to do so. - Allows removal of Reset button and Help functionality from DateTimePicker. - Adds PublishDateTimePicker to block-editor. This exposes a specialised DateTimePicker that is good for choosing a post publish date. * Add deprecated warning for when __nextRemoveHelpButton and __nextRemoveResetButton are not given * Target component instead of classname * Use space() instead of hardcoded widths * Update CSS selectors used in E2E tests * Update changelog * Update scheduling e2e test * Use relative URLs in README.md
What?
Implements the new design for the Publish popover (
DateTimePicker) that's in #39082.Since we're changing most of the CSS anyway, I've taken this opportunity to convert most of
DateTimePickerto use existing WordPress components and Emotion styling.Why?
Part of #39082.
How?
DateTimePicker,DatePickerandTimePicker.DateTimePicker,DatePickerandTimePickerto use existing WordPress components and Emotion styling where it makes sense to do so.react-datestheming as CSS. Emotion doesn't really add much here since we have to use CSS selectors anyway.DateTimePickerfolder structure to be more consistent with newer components.date-timenow has its own folder.DateTimePicker.__nextprop. We should eventually make this the default.PublishDateTimePickerto block-editor.DateTimePickerthat is used for changing a post publish date.Testing Instructions
Also play with the
DateTimePickerstorybook innpm run storybook:dev.There are existing unit tests for
DateTimePickerwhich hopefully I didn't break.Screenshots or screencast