Button: Add hover style to secondary variant
#67325
Conversation
jameskoster
added
[Type] Enhancement
|
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. |
|
Size Change: +58 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
If we do it for the secondary button, we should probably also do it for the primary and tertiary buttons, since conceptually they are the same, buttons that carry with them an implicit action:
Conceptually the toggle buttons used for Styles, in the above, are not the same, though, they have more relation to the segmented control group, or even (a stretch) with tab-panel navigation. They should probably not have their weight changed. Right? |
|
The hover style changes are great improvement. Thanks @jameskoster! I’m not sure the font weight change is necessary. It’s pretty much unnoticeable when paired with the addition of
If we go ahead with the |
Currently they're secondary buttons with style overrides, so they will inherit and changes to the Button/Secondary component. Clearly this is a bit fragile and less than ideal. Semantically the behavior of this UI is more like a radio group, or Since they already utilise style overrides, I suppose we should add one more to preserve the font weight, with a view to circling back to explore a refresh. It would be nice if they somehow included a preview, a bit like these: 
What do you think? |
|
@matt-west the font change is about introducing another differentiator to help users with visual impairments like color blindness distinguish buttons from inputs. The centrally aligned text in Buttons helps, but there can be situations where that is not apparent, e.g.: 
I'll push a change to apply the increased font weight to |
|
@jameskoster My observation was that it’s difficult to see the font weight change between the current and updated versions of the button. (The perceived letter spacing is impacted by adding I appreciate that I’m using a high-resolution monitor though, so the difference may be more pronounced for others. The motivation for introducing the font weight change is sound. So feel free to ignore my comment if I’m the only one seeing this. Screen.Recording.2024-11-28.at.16.52.13.mov |
|
Ah, thanks for explaining. Curiously that's quite different to what I'm seeing: 
I presume disabling |
Yep, that’s correct. Here’s what I see with Screen.Recording.2024-11-29.at.08.57.17.mov |
|
I removed the antialiasing, I suppose that's a detail to discuss at a higher level (IE should it be applied globally)? |
This has been a long discussion in the past, where it was decided that WordPress should not change the default antialiasing. This is an accessibility consideration, because different platforms (Mac, Windows, Linux, Android, etc) use different techniques to enhance legibility. Of course that conversation can be revisited, though I don't know the situation has substantially changed since the last time. |
|
Flaky tests detected in 64a84d0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12085153167
|
|
This seems like a solid change for the entire system. Should we update the title so that it reflects all button styles have been updated? Thanks for the heads-up! |
|
All good on the A4A front. Thanks for keeping us in the loop! |
jameskoster
changed the title
secondary variant
|
I updated the title 👍 Having changed the weight for all variants, we'll need to smoke test the software since there are one or two instances of non-standard button styles, where the increased weight might not be desirable, e.g.: 
|
There was a problem hiding this comment.
Here's a diff from the variant matrix. I can see the weights are increased in all variants expect default and link. Just checking that the omission of default from the weight bumps was intentional.
| Before | After |
|---|---|
 |
 |
- I noticed there is a hover style bug in
secondary+isPressed, which occurs at least once in our codebase. - Hover
coloris darkened insecondarybut not intertiary, although the hoverbackgroundis the same color for both variants. Is this intentional?
|
It's very important when testing this to not look just at individual buttons in isolation, but looking at the whole UI together. If things still work there, that's when we can move forward. I'm personally not entirely sure, tending to agree with @matt-west that perhaps the difference isn't that meaningful. Can defer! |
|
Another example of button font weight leaking into other UIs: 
|
Nope, I'll address those points :)
I suspect the difference is most meaningful for folks with visual impairments. For my subjective part I find it a bit easier to identify buttons (especially |
I pushed a fix for this, but if there's a preference for a different approach let me know :) |
|
What's the consensus here @gutenberg-design @matt-west? For me it's a marginal a11y improvement that doesn't have any negative impact. But if there are strong objections that's fine too. If we're keen then I'll do the work to adjust the weight in all the peculiar button instances outlined above. |
|
I'm not too sold on it, after testing it in practice, the marginal aspect of it doesn't seem meaningful in solving the issues outlined. |
|
Do you mean just the weight change, or the hover styles too? The latter seems quite important to me. |
|
The hover styles are good. It's the weight change, because of how much it affects, I don't think the marginal improvement is worth it. |
|
The hover state for the secondary action is needed and I like how the subtle background color aligns with the hover state for the tertiary variant, thanks for working on this @jameskoster ! I agree that the change in font weight is likely not enough of a visual indication to make a difference, and I find the corresponding change in button width on hover a bit distracting/noisy. |
|
I removed the weight change 👍 |
Co-authored-by: Lena Morita <[email protected]>
Co-authored-by: Lena Morita <[email protected]>
jameskoster
changed the title
secondary variantsecondary variant
What?
Update the appearance of
secondaryvariant of theButtoncomponent.tertiarybuttons for now).Why?
This is outlined and discussed in #64744. Relevant pieces:
Hover style
Increased font weight
For folks with certain visual impairments the current appearance is very similar to inputs. A small increase in font weight helps distinguish one from the other.
-webkit-font-smoothing: antialiasedis applied to assist with legibility. This may be a separate consideration, and it may make sense to create a mixin so that it gets applied in all instances of medium font weight. Let's discuss that.Note: We might also consider applying this to primary/tertiary buttons too, if there's a concern about consistency within the family of
Buttonvariants.To test
In Storybook:
npm run storybook:devsecondaryvariant of theButtoncomponentIn the Editor: