-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Post Date: Allow user to pick Site Default, a suggested date format, or a custom date format #39209
Conversation
Size Change: +1.12 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
...[ | ||
_x( 'M jS', 'date format option' ), | ||
_x( 'M j, Y', 'date format option' ), | ||
_x( 'M j, Y, h:i A', 'date format option' ), | ||
_x( 'F j, Y', 'date format option' ), | ||
_x( 'd/m/Y', 'date format option' ), | ||
].map( ( suggestedFormat ) => ( { | ||
key: suggestedFormat, | ||
name: dateI18n( suggestedFormat, date ), | ||
} ) ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably an issue in trunk too, so it might be something to fix separately, but thought I'd mention it as it might be a fix we can roll in to this PR.
The labels could be improved for screenreaders. Both 'Feb 25, 2022' and 'Februrary 25, 2022' are announced as 'February 25th 2022' using Voiceover. I it does some normalisation of dates. I'm not sure if we can add some screenreader text into the CustomSelectControl
items that also contains the format.
Let me know if you want me to create a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting. That's tricky. I'm not entirely sure what the expectations would be for folks using screen readers. Go ahead and create a new issue so that we can ask for feedback on that from a11y folks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a new issue - #39278
Testing again, and there's also no way to tell which item in the dropdown is currently checked. 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a quick look at the SelectControl component, and it looks like it doesn't currently set the selected
attribute on option elements (here). I was wondering if that could be the culprit? (Probably something to look at separately to this PR?)
I thought some more about what formats to suggest to the user (see How I arrived at the suggested date formats in the description) and made some changes there. I also added the ability to set a custom format (see updated video in the description) which I think is necessary as we can't cover every date format that a theme author might want in the dropdown. Keen for feedback! Particularly on my thinking around what date formats to show. Internationalisation makes it very tricky. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great to me @noisysocks, thanks for the excellent PR description and explanation of the decisions involved! I particularly like being able to edit the custom date format in realtime in the editor UI. While this is an advanced control, I agree, it'd be really great to be able to open up this level of control to themers, and keeping it hidden behind the Custom setting means that we're not unintentionally exposing it to novice users 👍
In a future follow-up, it'd be fun for us to explore a WYSIWYG interface for dragging/dropping formatting for dates, but that exploration is for another time 😃
One thing I noticed, since you mentioned i18n, is that as an Australian user of my US English test site, it isn't immediately noticeable from the formatting suggestions if a number is day or month. I'm not sure if there's anything we can really do easily for it at this stage, but just thought I'd mention it:
Yeah, this is probably the only way to not have to make any compromises. It's obviously a bit of work to implement such an interface, though. I'm not sure if the cost vs benefit stacks up just yet.
We could change the date in the dropdown to use a sample date with a day greater than 12. For example, 20221-01-25. I'm not sure if this is more unclear, though. It might lead users to get confused and think that their post's publish date is the sample date. What do you think? |
Another reason to do this is that if we're using the post date it's not always clear what the difference is between the medium date which abbreviates the month name and the long date which doesn't. For example, if the post date is in May, it won't be clear because May is already three letters. I'll switch to using a sample date. We can change our minds later. |
Agreed, for the moment we probably need a stronger use case for date formats being frequently edited to make it a worthwhile investment of time in building it.
I think the sample date approach sounds like a good way forward to me! |
I moved the dropdown into its own component and am using that in the Comment Date block now too. Easiest way to test that is to edit a post, edit the template (Post inspector → Template → Edit), and then insert a Comments Query Loop block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing really well for me @noisysocks, thanks for re-arranging things and adding in the extra test! Just found a tiny formatting issue in the documentation where I've added a comment.
✅ Works with site's datetime formatting settings
✅ Post Date and Comment Date blocks work correctly with the new component
✅ Suggested and custom date formats work as expected, and escaped characters can be used to inject a word like "at":
✅ Custom date formats are matched against the list of suggested formats, so on editor reload if there's a match, the suggested format is highlighted if there's a match, and if not, the custom field is pre-selected
Thanks again for the detailed testing notes / explanations, the code change LGTM! It'll be good to get some design feedback on the suggested formats, too — they look like a good starting point to me, though I was wondering what the preference would be between capitalised / not capitalised AM/PM / am/pm?
packages/block-editor/src/components/date-format-control/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/date-format-control/index.js
Outdated
Show resolved
Hide resolved
Thanks for the review! I definitely want to get some design feedback on my thinking in the description and the list of suggested formats before merging.
From my research this seems to be locale specific. Or at least that's how Apple has implemented it. American English uses "AM", UK English uses "am", and most other languages use 24 hour time. I appreciate that that's all pretty academic though. It's difficult to strike a balance between having suggestions that are correct for the site's locale and useful for theme developers while also not having hundreds of options in the dropdown. Again, feedback welcome 😅 |
Thanks for the explanations, I didn't realise it varied so much by locale! That all sounds reasonable to me, and I quite like the defaults you've added. Let's see what the design experts think 😀 |
I think all the decisions here make sense. I like the presets, the idea to leverage localization, as well as the use of a sample date to ensure that the previews are clear. I like the Custom functionality too, but I'm wondering if we have that UI pattern (selecting custom in a dropdown to show an additional field) in place for other blocks anymore? I think the closest we have at the moment is the custom field for font size, which uses a slightly different paradigm: That doesn't feel quite right here though, since I think it would over-emphasize the custom option. I'd like one more designer to take a look and weigh in. @jasmussen or @jameskoster perhaps? |
Great suggestions ❤️ thank you! |
|
||
let postDate = date ? ( | ||
<time dateTime={ dateI18n( 'c', date ) } ref={ timeRef }> | ||
{ dateI18n( resolvedFormat, date ) } | ||
{ dateI18n( format || siteFormat, date ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need a hook somewhere to ensure siteFormat
is available.
For me at least, there seems to be a race condition whereby, if I refresh the Post Editor with a Post Date block and the site default selected, both format
and siteFormat
are undefined
.
This is only in the Post Editor. The Site Editor works as expected.
The result is, when refreshing the page, format() complains that it cannot find a length.
Adding a new Post Date block reveals that the value is indeed returned at some point after refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah bugger. I guess it happens while the site entity is loading.
I added a fallback to the @wordpress/date
format in b3fcfda.
Let me know if that fixes it for you.
packages/block-editor/src/components/date-format-control/index.js
Outdated
Show resolved
Hide resolved
ce9835d
to
c9d954d
Compare
Alrighty I swapped the toggle group for a regular toggle component, like in #39209 (comment). I think it definitely is clearer now that "custom" means only one thing. I'll update the video in the description. @jameskoster: In the design you had two labels for the toggle but unfortunately that's not how our toggle component works. Toggles only have one label which is on the right. I could add text above the toggle but it wouldn't work like a regular label (clicking it should focus the field) and it'd probably confuse assistive technology users. I worked around this by using "Default format (March 27, 2022)" for the copy. We could also put the date on a new line which I think might look a little nicer. Happy to hear other suggestions. |
Yes correct! I think it feels a little disconnected though: I don't think there's a problem with the spacing, it's just that the help text usually shows a sentence or two of copy: I pushed 08d3e41 which moves the date to a new line under the label, which is my preference: I'd love to get this merged and do any further iteration in follow-up PRs! What we have here is a lot better than what's in |
Good catch. Fixed in d6cc73e. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works really well, great job iterating here.
This is a good improvement over what's in trunk
, so I really think it's worth merging and then following up with any further design changes.
As an aside, I noticed another bug with the formats while testing, but it seems like an existing issue with the dateI18n
utility rather than anything introduced here, so I've made an issue for it - #39567.
Nice one, thanks for working on this. Let's remember to follow up on the help text, so this isn't the only place where it's used like this: For lack of larger improvements such as those Jay suggested, we could start with something simple, like this: CC: @ciampo, I think there's some margin between toggle and help text we need to tweak so it's smaller. More specifically, when a toggle has helptext, the toggle itself doesn't need to have bottom margin, it appears, since the helptext itself has that. |
Good catch! @mirka , can this also be part of the recent style "normalisation" treatment that you're applying across components? |
Yes, that will fit in perfectly with the work in #38730 ✨ @jasmussen I'll ping you on the PR for a check when we do |
…or a custom date format (WordPress#39209) * Post Date: Allow user to pick Site Default, or a suggested date format * Update suggested formats to be more localizable * Add support for custom formats * Move is12Hour check to function * Distinguish between 'long' and 'full' like Apple does * Use a sample date in the dropdown so as to better illustrate the formats * Split out DateFormatControl and update the Comment Date block * Add basic tests for is12HourFormat * Fix markdown link formatting * Combine 'Format settings' and 'Link settings' into single 'Settings' panel * WIP * Use Default / Custom radio control Re-jig DateFormatControl into DateFormatPicker. Users now select either Default or Custom in a ratio control and, then, if they select Custom, they can select a suggested date format or enter a manual one. * Simplify suggested date formats * Use date and time formats from @wordpress/date as a fallback * Use 'Enter your own date format' * Explain what null means in the doc comment * Use ToggleControl for selecting a default date * Use __experimental like a coward * Move example of default format to new line * Fix 'Link to post' label
Description
Closes #38751.
Kapture.2022-03-11.at.12.39.06.mp4
Changes the Post Date block so that you can choose between using the site's default date format, a suggested format, or a completely custom format.
How I arrived at the suggested date formats
Which formats should we suggest? is a difficult question as different locales have different date formatting conventions.
In my opinion there are a few requirements:
We can meet requirement 3 by allowing the theme developer to type any arbitrary date format into a text field.
If we pick a small set of suggested formats, as in #38751 (comment), then we will meet requirement 2 but struggle to meet requirement 1. This is because any list we come up with is bound to not be appropriate in some locales. For example, 4/20/22 is typical in US English but 20/04/2022 is typical elsewhere. Or, "April 20, 2022" makes sense in US English but this would be "20 April 2022" in UK English and "20. April 2022" in German.
We could flood the dropdown with hundreds of different options so as to cover all locales and meet requirement 1, but then this would be an overwhelming amount of options and therefore not meet requirement 2.
So what I've opted to do is to start from four categories: short, medium, long, and full. This is inspired by what Apple has in System Preferences. For the short, medium, and long categories, I added the option of showing the time as well. This gives seven options. Each option is localised depending on the site's language. That is, the suggested formats will change depending on what language WordPress is set to.
en_US
en_GB
de
Notes
null
attributes.format
is what denotes that the user has selected Site default. This is the default.attributes.format === siteFormat
because the user intended that or if it's just a coincidence.@wordpress/date
settings as we can get everything we need from/wp/v2/settings
(thesite
entity).I need to update the Comment Date block with the same changes. (Maybe the dropdown should be a block editor component? (Or a block supports? 😬))is12Hour
calculation and fixed the logic of this. Previously it was only looking fora
but technically a 12 hour format string could just be e.g.g:i
with noa
.Testing Instructions
Checklist:
npm run lint
, Guidelines: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/ -→*.native.js
files for terms that need renaming or removal). <!-- React Native mobile Gutenberg guidelines: https://github.com/WordPress/gutenberg/blob/HEAD/docs/contributors/code/native-mobile.md -→