-
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
Block Library: Add features to the Post Date block. #19857
Conversation
The pencil icon leaves a lot to be desired. Its not at all obvious, and actually seems to imply "change" or "edit" — not "format". Maybe we can replace it with a label: I wish there was more control over the date and time formatting. Combining them both into a single selection reduces the control — or, if we add more, requires more options to provide more control. If we were to separate the formatting we could offer more control without having to list a dozen different combinations: I found it jarring at first that selecting the block instantly shows the calendar to change the date. I wasn't expecting it, and I didn't really understand why that control was showing. Part of me thinks it should be something more intentional, like a "change" button in the toolbar: |
Using labels in the toolbar like that is not a good idea because
localization can break the layout.
|
Sure, I can see how translations could cause troubles here. But I also don't think a pencil icon is a good way to indicate "change the date and time formatting". I'm not sure such an icon exists. We had a similar problem for the Image block, and ended up going with a "Replace" label after trying a handful of different icons. |
Agreed. I suggested we use a calendar icon with some sort of pencil/tool
overlaying on top.
|
The block icon itself is already a calendar, and I worry about the repetition. Also, would this icon be for changing the date/time formatting, or editing the date of the post? Here's a few quick variations of the calendar, pencil, and clock icons: I don't think any of them are better or more obvious that using a label for the formatting. Generally, combining multiple icons at such a small size is not recommended — both icons tend to lose their "shape" and become unrecognizable. I could, however, see the pencil icon working for changing the date of the post. |
How about the pencil for changing the date and a calendar or placeholder
date with a gear icon on top for the format.
|
</Popover> | ||
) } | ||
<BlockControls> | ||
<ToolbarGroup |
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 don't think the format is changed often enough to warrant a placement in the toolbar. It could be just an inspector control.
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.
3d24007
to
2e813e4
Compare
I've refactored the opening of the calendar popover so that it now opens through a pencil icon on the toolbar. I also moved the format dropdown to the inspector. This solves the icon problem for now. @shaunandrews I think this is ready to go. |
Size Change: +5.86 kB (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
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.
Date/time selection seems to work, but I'm not sure how to change the format:
Edit: Ohhh, it's mentioned here: #19857 (comment)
I also moved the format dropdown to the inspector.
Will test the format now.
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.
Code-wise, this is looking good.
I have some UI concerns, but it's probably okay to address those in a future iteration.
We should avoid the pencil as the "edit" icon. |
const is12Hour = /a(?!\\)/i.test( | ||
settings.formats.time | ||
.toLowerCase() // Test only for the lower case "a". | ||
.replace( /\\\\/g, '' ) // Replace "//" with empty strings. | ||
.split( '' ) | ||
.reverse() | ||
.join( '' ) // Reverse the string and test for "a" not followed by a slash. |
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.
- Why reverse the string and not use a non-capturing group like
/(?:[^\\])a/
? - If the RegExp already has the
i
flag, why pipe through.toLowerCase
?
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 now see that the original occurrence came with 256badd, three years ago, so ignore this message.
Closes #19697
Description
This PR implements the design from #19697.
How to test this?
Verify all the features from #19697 work as expected with a Post Date block inserted in a regular post.
Screenshots
Types of Changes
New Feature: The Post Date block now has a more rich editing experience and allows you to customize the block-specific date format directly.
Checklist: