Simplify Button styles and remove isTertiary.#25632
Simplify Button styles and remove isTertiary.#25632ZebulanStanphill wants to merge 1 commit intotrunkfrom
Conversation
ZebulanStanphill
added
[Type] Enhancement
|
Size Change: -235 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
ZebulanStanphill
requested review from
Soean,
ajitbohra,
chrisvanpatten,
ellatrix,
talldan and
youknowriad
as code owners
afercia
left a comment
afercia
left a comment
There was a problem hiding this comment.
Thanks @ZebulanStanphill !
From an accessibility perspective I'd strongly recommend this change, as it fixes what the accessibility team identified as an accessibility regression in WordPress 5.5 (see the issue #23890).
In the interest of collaboration between teams and for the greater good of the project, I do realize this needs design feedback 🙂 Approving from an a11y perspective.
|
This looks great from my perspective, too; although I recognize that I'm basically just seconding @afercia's accessibility approval. ;) |
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
Let's get this a proper design review, I'm pretty sure this is a regression in terms of the buttons importance hierarchy
|
Thank you for working in this @ZebulanStanphill! I understand the need to address #23890, but putting my designer hat on I have some different feedback. At first glance, these buttons look like input fields to me. You can see how there's little to no visual difference between the proposed style for buttons and the 'Add tags' field:
This is also affecting the hierarchy of buttons and options. While this may not be an issue when there are just a couple of buttons, I find it very hard to parse something like this:
We need a balance that makes it clear these are buttons but that also not overwhelm a whole other set of users. We're also not accounting for visual inconsistencies across the UI, like icon buttons:
or disabled states like the 'Redo' button over here:
Overall I think your exploration is incredibly useful, as it allows us to find these nuanced cases that we need to be aware of when exploring a solution. |
|
The three button styles seem to follow Material Design: https://material.io/components/buttons#hierarchy-and-placement I understand the PR from a11y perspective to identify every button directly as a button. However, I don't think merging the second and third level is the way to go... Edit: @enriquesanchez was faster and has a much better answer. :) |
|
The points about icon button and disabled button styles are both very valid. The quick answer would be to just add borders to all of them, but in the case of icon buttons, that seems a bit too much in the top bar. Thinking about it some more, I think there needs to be two different classes of button styles: standard and toolbar. Standard buttons would always have an outline, while toolbar buttons wouldn't, since their usage in a toolbar should be enough to make it clear that they're buttons. How does this sound? There's a bit of a problem with the top bar, however: it's actually not a toolbar. Or at least not all of it. The left half is a toolbar, but the right half isn't. As for buttons looking like input fields, that's another good point. But both buttons and input fields ought to have clearly defined borders. So we need to find a different way to distinguish the two via different shaping. Any ideas? @mariohamann It's worth remembering that Material Design patterns were not designed with accessibility in mind. The problem with using buttons that consist only of accent-colored text is that accessible designs must account for colorblind users. There needs to be distinct contrast and shaping for all variations. Again, there must be a recognizable difference in shape/contrast. Additionally, blue-text buttons look a lot like links, which they aren't. |
+1 to this. Keeping using Material Design as a reference in a project that aims to be accessible isn't that productive 🙂 |
|
Agree its not productive to compare to Material in this case, so let's just stick with the issue we are trying to solve. The buttons currently look like plain text but this particular solution will have them looking like input fields (and introduces additional a11y issues as brought up by @enriquesanchez.) Issues that I think are blockers. I think this was a solid attempt but needs to go back to the drawing board to make the UI both accessible and intuitive. (Underlines immediately comes to mind but am open to other options.) |
|
Agreed, this needs some more discussion. Let's continue in #23890. If anyone has any ideas, please share them there. |
ZebulanStanphill
added
the
[Status] Blocked
I do understand this concern. @enriquesanchez as designer and also accessibility team member and representative, do you have any concrete proposal to find a better balance? |
youknowriad
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
|
Since it's considerably out-of-date, and there's no final design solution in sight, I'm closing this PR. I still think there's a need for a clearer and more consistent visual distinction between links, buttons, and I should also note that I may have gone overboard with putting outlines on every text button. I think the ones in toolbars can probably go without them, since they're known to be clickable by context, in the same way the icon buttons are known to be clickable. Please continue discussion in #23890. |
9 participants
Description
Fixes #23890.
This PR attempts to improve the accessibility of the
Buttoncomponent's styles. In general, I tried to clean up some of the messy CSS and make everything more consistent.The accent color is now used more consistently on buttons: it is now only used for hover, focus, and primary styles. (With the exception of
isLinkbuttons, which I haven't touched in this PR.) I always thought it was confusing how the accent color was used as the default outline color of some buttons, but the hover outline color of others.I've deprecated
isTertiaryand turned it into an alias ofisSecondary(similar to what happened toisDefault). I couldn't find any rhyme or reason as to when one would be chosen over the other, so I figured it would make sense to just merge them.How has this been tested?
I've checked just about every button I could find in the UI, both with "Show icon labels" on and with it off, to make sure there are no broken/missing styles.
Screenshots
Before
After
Checklist: