Global styles: split styles menus in revisions and everything else #51318
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
commented
Jun 8, 2023
| @@ -179,3 +179,14 @@ | |||
| .edit-site-global-styles-sidebar__panel .block-editor-block-icon svg { | |||
| fill: currentColor; | |||
| } | |||
|
|
|||
| [class][class].edit-site-global-styles-sidebar__revisions-count-badge { | |||
There was a problem hiding this comment.
ramonjd
changed the title
ramonjd
added
[Type] Enhancement
|
Size Change: +151 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
update/styles-drop-down-menus
branch
from
259b5da
to
dd65058
Compare
ramonjd
commented
Jun 8, 2023
ramonjd
force-pushed
the
update/styles-drop-down-menus
branch
from
64864ec
to
01118a2
Compare
ramonjd
changed the title
ramonjd
commented
Jun 9, 2023
| </span> | ||
| ); | ||
| } | ||
| function GlobalStylesRevisionsMenu() { |
There was a problem hiding this comment.
Maybe we could extract these dropdowns into a new file or files.
|
Flaky tests detected in 01118a2ff7b51e04a3c0659f8ee100d7cceba20d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5216973427
|
ramonjd
force-pushed
the
update/styles-drop-down-menus
branch
2 times, most recently
from
066c649
to
b5d8ecb
Compare
There was a problem hiding this comment.
This looks cool! It's generally working well for me, but I noticed that if I switch my editor settings to show button text labels, then the labels break out of the header area:
Could one solution be to simplify some of the label text? I.e. possibly something like Revisions and More (or just Actions) instead of Styles revisions actions and Styles actions? That might need some design feedback on the wording, potentially.
Thanks for testing this out @andrewserong 🙇 I agree we could simplify the labels. I'm just wondering if we'd have to make the context clearer, e.g., adding a role/aria label to the surrounding header container like Here's what it looks like in English with simplified copy: 
It's better, but it doesn't really solve the stacking 🤔 Another consideration is that other languages might use more characters to translate these strings. I think a decent alternative could be to group menu items:
cc @WordPress/gutenberg-design |
|
Short answer: keep the labels as short as you can. Can you fit Longer answer: there's a great deal of compounding issues as a result of the text-labels-only option, and perhaps that feature was landed too soon since it doesn't well handle wrapping cases like these. I'll put on my todo list to, as soon as possible, revisit the behavior with some mockups that handle wrapping. I imagine through concise labels (even text buttons can have more verbose tooltips), massaging font size, and popping off items into dropdowns when things wrap, we can get the feature into a good place. In the mean time, if we are comfortable shipping the following in trunk, I don't think we should let the wrapping inspector controls block this PR from landing: 
The goal of the labels only mode must surely be to increase legibility, and until that's considered in a holistic way, any fixes we apply locally will just be bandaids that add technical debt. We need to make the componentry smart enough to handle these cases automatically. |
|
Generally we're leaning towards placing revisions access at the bottom of these panels. For more context see the 'Footer' section in #50379, plus #49597. I wonder if we should try that here too, as suggested in #51203:
It would solve the text label issue, and the over-crowded icon buttons in that top-right corner of the browser. Side note, I'm not sure the icons added to the Custom CSS and Welcome Guide menu items are all that helpful. |
I've updated the label copy. Doesn't quite make it... 
No worries! Agree.
👍 Removed the icons.
Looks good. Maybe we could iterate? I think @matias's intention was to also group in the reset functionality. The fastest thing I could achieve in the following (because we have an existing slot). 2023-06-15.11.14.36.mp4Otherwise we'd need tool around with the entire sidebar flex settings (?) |
|
FWIW resetting is still grouped in – you enter Revisions and select the original one then apply. Probably fine to keep the dedicated shortcut in the ellipsis menu too, alongside other actions (add css).
I wasn't aware there was a prescribed design direction for this. If that is the case then you can probably ignore my feedback :) |
Thanks for the feedback again! We could build a reset to theme default in eventually (it doesn't exist yet). I had an earlier commit in the original PR that I kept which does that, so it's in the vault for when we decide to come back 😄 |
There was a problem hiding this comment.
Good points in this discussion about the wider issues surrounding the "show button text labels" mode. For now, I think it might be possible to get all the buttons lined up on the one row without too much hacking (in English, at least), if we remove the FlexItem that's wrapping <GlobalStylesMenuSlot /> in global-styles-sidebar.js (around line 83). Here's how it looks if the slot is a sibling to the other FlexItems:
ramonjd
force-pushed
the
update/styles-drop-down-menus
branch
from
d1ea5c6
to
dddfe82
Compare
Interesting. Thanks @andrewserong I've updated this branch with those changes. Checked in FF, Chrome and Safari. LGTM 
|
andrewserong
approved these changes
Jun 20, 2023
…els" preferences mode
ramonjd
force-pushed
the
update/styles-drop-down-menus
branch
from
d82512b
to
9426264
Compare
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…ordPress#51318) * Switching items in the dropdown in GlobalStylesMenu to MenuItems * Icons and stuff * Simplify aria labels on style actions menus and add role to surrounding header. * Grouping menuitems * Removing icons and shortening the copy for text view * Need to close the menu after some actions * Remove flex item so that the text labels sit more neatly in "text labels" preferences mode * changelog * Update e2e selectors
This was referenced Feb 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR:
n” action buttons into separate dropdownsbackup) to the styles header buttons rowTesting Instructions
Screenshots or screencast
2023-06-15.10.59.06.mp4