Image block: Add support for "more" dropdown for additional tools in Write mode #66605
Conversation
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
mcsf,
oandregal,
jasmussen,
richtabor and
t-hamano
| attributesToDisplayControls.length, | ||
| accessibleWhenDisabled: true, | ||
| // TODO: update this one.. | ||
| description: __( 'Displays more tools…' ), |
There was a problem hiding this comment.
We need to find a proper description here.
There was a problem hiding this comment.
I'd remove the ellipsis, but other than that this matches what we do FormatToolbar, which has been worked on by a11y experts.
We could try to be more descriptive (i.e. find something accurate and concise like "display more content-editing controls", etc.), but in the end couldn't that verbosity get in the way of the user? The sooner they'll expand the dropdown menu, the sooner they can get a true sense of what hides in there.
(Precise and verbose descriptions can also get in the way of translation.)
|
Size Change: +245 B (+0.01%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
Hey, thanks for tackling this. But I'm seeing this:
I think we might need some input from @richtabor as he had some input on #66455 as well, and I want to make sure I'm not missing nuance. But the way I see it, the items in the "More" dropdown should never add block toolbar options, and the items Title and Alternative text should never show up in the block toolbar. The options could potentially open modals, or otherwise, where you can input the values you need. But the purpose is to simplify the block toolbar, not to add conditionals. |
|
I posted an idea here: #66455 (comment) |
Apologies for missing that. I love that idea. Though one bit of feedback, I would add a vertical separator between "Replace" and the "More" dropdown. Conceptually the "More" dropdown is grouped with the "additional" tools like Link, just like it would be for a Paragraph block, so there needs to be that vertical black line separator still, if there's no link button. |
That what I got from the issue screenshots.. I'll see to update with the suggestions. |
| attributesToDisplayControls.length, | ||
| accessibleWhenDisabled: true, | ||
| // TODO: update this one.. | ||
| description: __( 'Displays more tools…' ), |
There was a problem hiding this comment.
I'd remove the ellipsis, but other than that this matches what we do FormatToolbar, which has been worked on by a11y experts.
We could try to be more descriptive (i.e. find something accurate and concise like "display more content-editing controls", etc.), but in the end couldn't that verbosity get in the way of the user? The sooner they'll expand the dropdown menu, the sooner they can get a true sense of what hides in there.
(Precise and verbose descriptions can also get in the way of translation.)
|
Appreciate the review but let's not merge this, I think Aki is on to something with his idea though! |
|
I followed the suggestion by @t-hamano eventually. It's similar to some RichText controls. |
ntsekouras
force-pushed
the
add/more-menu-content-only-image-block
branch
from
0a93c24 to
68e5deb
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in bd10056. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11614579135
|
|
Seeing this:
Nice work, toolbar-wise, this hits the mark in terms of appearance, functionality, etc. The now open question is the "toggled states" inside the dropdown. It's not clear to me those are useful at all. What purpose do they serve, can can we just remove those states, so that the menu items are just menu items? |
I removed them. |
|
Happy to ship this from my end 👍 👍 |
| <MenuItem | ||
| onClick={ () => { | ||
| setIsAltDialogOpen( true ); | ||
| onClose(); | ||
| } } | ||
| > | ||
| { _x( | ||
| 'Alternative text', | ||
| 'Alternative text for an image. Block toolbar label, a low character count is preferred.' | ||
| ) } | ||
| </MenuItem> | ||
| <MenuItem | ||
| onClick={ () => { | ||
| setIsTitleDialogOpen( true ); | ||
| onClose(); | ||
| } } | ||
| > | ||
| { __( 'Title text' ) } | ||
| </MenuItem> |
There was a problem hiding this comment.
As these open another popover they should probably have aria-haspopup - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup.
When pressing escape from the alt or title popovers, focus doesn't go back to these buttons, which is a bit awkward and it's not really clear what happened, was the change successful or not? A user has to reopen it to find out if the changes stuck which then takes two presses.
There was a problem hiding this comment.
I added the aria prop here.
Regarding the design/UX questions I'd also defer to @jasmussen or @jameskoster . It's fairly new so it would need a bit time to see if we have some different feedback from users.
While focus doesn't go back to the button that triggered it, there is no focus loss though. The use case you describe requires two clicks but I'm not sure if someone who writes something and clicks escape goes back to a dialog to see if it worked - it should have worked, no?
Finally I followed some RichText tools approach (which was the same as the suggested design to close the main menu and open a dialog). That doesn't mean that it might be wrong, but if it is we should update that too.
There was a problem hiding this comment.
I've no strong opinions on the behavior, full-modal or otherwise, and would support whichever solves the problem.
|
When an image is used in a synced pattern as an override, it looks a bit unusual having a more menu at the start of the toolbar on its own: The menu does lose a bit of context when there are no other options in the same group, it took me a while to realize what was happening. 🤔 It will get a bit better in the future if block bindings/pattern overrides can support the link option, but I'm not sure where that support is at right now. |
It's not clear how much control we have over these contentOnly toolbars, but specifically from the perspective of that Image block, I would think of Upload being in the same group as the "More" control. I.e.—rough mockup—like this: |
@jasmussen I had a go at moving the upload button. It is possible, but not straightforward because of the way slot fills work (same issue as #15641). Though a lot of the time these images might not have upload button either if they're from the media library: Let me know what you think and if it's worth following up on. |
|
If the code isn't too ugly or problematic, and doesn't take too much of your time, I think it would be a good move 🙏 |
…Write mode (WordPress#66605) Co-authored-by: ntsekouras <[email protected]> Co-authored-by: mcsf <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: t-hamano <[email protected]>
What?
Resolves: #66455
Testing Instructions
writemodeScreenshots or screencast
Screen.Recording.2024-11-11.at.12.02.02.PM.mov