Display labels instead of icons in top toolbar. #24304
Conversation
tellthemachines
added
[Focus] Accessibility (a11y)
tellthemachines
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
|
Size Change: +2.19 kB (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
|
Thanks for this PR! I spent some time working with the previous design explorations and wanted to present my further iterations here. While this isn't direct feedback on the PR (yet), I thought it best to share on this PR for a discussion as to what path works best. Option 1 - Add labels under icons
Option 2 - Expand height of topbar
Option 3 - Smaller icons
Mobile
Option 4 - Text only
Mobile
FeedbackI wanted to get these early iterations up for discussion. Is there an option that feels better than the others? What else would you like to see iterated? If you're interested in more, check out my Figma file. |
tellthemachines
force-pushed
the
add/top-toolbar-labels
branch
from
e4317c0 to
8c9ec13
Compare
|
Here's my feedback on the current state of this PR: Text onlyI love the text only mode because it's so clean. But once the Top Toolbar option is introduced, it feels a little wonky. This PR chooses to keep the block toolbar on a second row, which could work, but would need updating to text only as well. (I think that's reserved for another PR) Top Toolbar modeThe bottom border is missing in the second level toolbar here:
Site iconI don't think we need to add text here. It just gets very out of place and messy.
Text edits
Settings buttonThis looks more like a button now than a toggle now. Should keep this pattern? Button sizesWhen both menu dropdowns are triggered, the "Add block" and "Publish" btns sizes are more visibly unequal and look like a mistake. I think they should all match sizes with this option.
Dropdown menus should not overlapThese dropdowns should appear next to their parent level menu. Example 1
Example 2
Settings item when in dropdownI think we should drop the "active" state of the Settings item when it's in the dropdown. More like this:
DropdownsI think we have a standard min-width on dropdowns like 258px. Let's apply that to these as well. After these changes, I'm happy to provide another review. 😄 |
|
I've left this in labels-only mode for now, we can always change it later if we decide it doesn't work. Or we could have three options: icons only, icons and labels, and labels only 😄 I've addressed most of @mapk 's feedback; all but the dropdown positioning as I'm still tussling with the Popover component on that front. Hopefully will have it fixed soon! The e2e tests are failing because of the changes I made to the label text on some of the buttons. I'd appreciate any feedback on those changes, so that I can fix the tests once they're sorted. |
|
Thanks for the quick turnaround, @tellthemachines! As you're working through this, I hope providing a bit more feedback is okay. If you'd like me to hold off until a certain point, just let me know. Top Toolbar modeI noticed that when selecting Top Toolbar mode, when I don't have a block selected the topbar bottom border doubles up.
Text buttonsWhen switching to a text only mode, I really think all the button interactions should work the same. It always felt odd to me to have icon buttons (ie. hover and pressed states) display differently than the text buttons, and now that these are all text buttons, it's even more obvious. An open question: Should the text only buttons reflect the same interactions that the Preview button has. These are no longer icon buttons, and should probably now reflect the interactions of our standard button states.
Settings and OptionsI just noticed that when both Settings and Options are in a dropdown, and activated, they both show blue. This doesn't feel right to me even though technically it is. Maybe fixing the whole button interaction thing will help this?
Icon dropdownIn text only mode, when the screen size is reduced, the buttons all move to a dropdown that is represented only with an icon. Does this make sense in the text only mode? Should we have a label for this icon?
There are so many small nuances that come to light as this gets developed, so I really appreciate your time with it all. |
ZebulanStanphill
added
the
Needs Accessibility Feedback
|
If we converted all the topbar text-only buttons to the default button instance as outlined in our Figma libraries, they would interact similarly to the Preview button. Check out this prototype below: (please ignore the black background)
|
Absolutely, all feedback helps!
Hmm, I can't reproduce this. Have you checked out my latest changes?
Very good question 😅 I've switched all the text buttons to behave like the Preview button in my latest commit, let me know if it works! Regarding the dropdown positioning issues, it's trickier than I thought initially. The Popover component that we use in dropdowns has very limited positioning capabilities. We have #21275 to address that, and from the looks of it it's going to require rewriting the whole component. I repositioned the Outline dropdown the best I could, but haven't been able to improve any of the others. Can we leave it as is for now, and revisit once Popover is rebuilt? |
After sitting on this one for a few days, I'm not fond of how our default button state looks for each of these topbar items. I iterated further with something more reminiscent of our Icon Button styles. It's not perfect, but the other direction felt overwhelming.
Next steps
|
tellthemachines
force-pushed
the
add/icon-labels-option
branch
from
5916692 to
8981e82
Compare
tellthemachines
force-pushed
the
add/top-toolbar-labels
branch
from
5d39850 to
76181c8
Compare
tellthemachines
requested review from
ellatrix,
nerrad,
ntwb,
talldan and
gziolo
as code owners
|
I got some feedback yesterday that moving to "text only" might be too much. The text only version loses hierarchy and grouping which can cause usability issues. I'm exploring a couple icon + text solutions to see how else we might be able to retain some sense of hierarchy and relationship once we introduce text into the topbar. |
tellthemachines
force-pushed
the
add/top-toolbar-labels
branch
from
1dfe264 to
52785f0
Compare
|
Caught some spacing issues on smaller screens.
|
|
Generally, the spacing in between these buttons all feels a bit off. These all feel too close.
These feel too far from one another.
Is there a way to make this more consistent across all the buttons in text-only mode? In this Figma mockup, I have them spaced 10px apart.
|
|
|
|
Noting that #24965 has been created to discuss improvements to the "Options" modal, which should help solve some of the overly-similar labels currently present. See also these other issues relating to the top toolbar:
|
|
@mapk Was wondering why 'Outline' was used for the button text that opens the List View, instead of 'List View'? Looking back through the designs in the comments, it looks like at one point it said 'List View' (#24304 (comment)) but then changed in the more recent designs. edit: Oh, I notice the tooltip currently says 'Outline' for the icon button, so that's probably why. I was wondering if we want to stick with List View in both places. |
|
@ZebulanStanphill, changing the spacing to 12px is totally fine. @talldan, No problem changing it to "List View". Let's make that happen. :) |
|
Glad to see this merged! Thanks everyone for the perseverance and I'd like to thank @nicolad who started the first attempt to build this in #15830 ❤️ (deserves props). Regarding this point by @ZebulanStanphill:
Ah! That's what the accessibility team has been asking in the last... years? Considering all the troubles for keyboard navigation to go from the selected block to the inspector and back, the teams asked several times to have the inspector "in place" close to the block instead of in the sidebar. Hopefully there will be new explorations in this direction. |
|
I'd like to propose props also for @tomjn who created the original issue, together with @nicolad who, as mentioned above, worked hard on the first PR. |
|
I actually raised the issue during a user group after a discussion with @mikelittle, he deserves the props :) |
Description
Fixes #10524 , along with #24234 . This PR points to #24234 so that only the changeset related to displaying button labels in the top toolbar is visible.
All the feedback and comments from #15830 have been addressed. One point that I'd appreciate further feedback on is the positioning of nested dropdowns, such as:
How has this been tested?
Tested in browser at different breakpoints. Keyboard navigation checked.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: