Fix hover/focus styles for style variation buttons
#50056
Merged
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
ntsekouras
added
[Type] Bug
|
Size Change: +1.23 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
I pushed a small tweak to make the shadow outset, and based on a mixin I hope we can reuse in general for focus styles like these. Looks like this now:
@mirka this style variation button style is also meant to be used in other places, such as here:
Could/should this be a variation of the generic Button component? How would that mainly work? Ideally we have as little local CSS as possible. I'd prefer not to block this PR as it improves up on what's already shipping, but I wonder if there's a more generic improvement we can make separately. |
jasmussen
approved these changes
Apr 26, 2023
|
I'm going to merge this, as it affects some styles from block editor components and not the |
The first question would be why these focus styles are deviating from the focus styles on solid buttons like
☝️ Focused primary button
☝️ As for these variants, I do not consider them to be the same as the "Explore all patterns" button (which looks like it's just the Secondary variant). The Style Variation buttons are specifically toggle buttons, which I assume is what justifies/necessitates this special color scheme. Do you agree with that assessment, @jasmussen? Is this UI pattern used anywhere else? |
I think the divergence is at least partially intentional — for inputs, secondary, and tertiary buttons, the surrounding border becomes the focus style, which is just thicker and blue. For primary inputs, since there isn't a border, the focus style has to have an inset halo of white, to be visible. If we can consolidate, let's definitely, though I'm not entirely sure how you'd do that.
We are using the gray-bordered style here as well: #50143 (review). The style is admittedly a little bit in flux, but in principle it's the same. It is possible we could consolidate them all into the secondary style, simply make the blue border gray by default. |
|
Semantically the variation selection UI is essentially a segmented control / radio. Should we consider using that component here instead?
|
|
What if there are 8 variations? They can be user created, after all. |
|
For sure, and that question can be aimed at the segmented control as well. For example if a theme provides 20 different present font sizes. How should it adapt when there isn't enough room to contain all the labels? Do they stack vertically, transform into a dropdown, something else? |
In that case the segmented control turns into a dropdown. In the case of the toggle buttons, I think the use case exists. The "Styles" panel is definitely not the greatest, as is, and could use a separate iteration. And to be sure, a segmented control would help communicate that style variations are meant to be mutually exclusive (Outline or Solid button, not both). But I also don't think either a segmented control is the right interface for this, as anything above 2 style variations already starts to break it. What other patterns have the clarity of toggleness that the segmented control has, but affords more text? |
|
We already deal with this need in More in general, I strongly echo @mirka's words: we should really try to consolidate styles at the component level, make sure we have a consistent UI language, and remove those style overrides in other packages. |
Appreciate this is veering into pedantry, but I'd suggest the segmented control is the right component, but the geography in this particular context makes the interface a bit awkward. IE the column is too narrow to accommodate more than 2-3 options. Perhaps we could investigate a layout variant on the segmented control component that arranges the values into two columns, and potentially disables the switching-to-dropdown behavior. Though personally I don't mind that, even in this case. Once you've chosen a variation there's no need for that UI to take up so much space. |
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?
Focus/hover styles for
style variationbuttons aren't the proper ones. This came up in: #49992, and this PR just updates them.Before
before.mov
After
after.mov