ToggleGroupControl: Improve styling for icon options #43060
Conversation
mirka
added
[Type] Enhancement
|
Size Change: +113 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
This is sweet! Here's before: Here's storybook:
I think the border-less and with-icons version of the segmented control will be useful, the focus style makes sense (since arrow keys are meant to pick the choice), and it even makes sense to replace the text decoration control since all those options are mutually exclusive. One thing that jumps out to me is that the toggled state is slightly rectangular: That appears to be because the height of the toggled state inside the current segmented control is also 30px (36px current size minus top/bottom borders, minus 2px inner padding top and bottom = 30px). Ideally the toggled state remains square, which would make it 30x30, but perhaps because this variant is borderless, we can keep the state 32x32? Perhaps not something to block this PR, since we should be optimizing for the future 40px size, which would make the toggled states inside 34x34:
But even in that case, it would be nice for the toggled states of the iconless version to be the full 40x40 sizes. Like so: (Speaking of, that group transform interface would also benefit from this control.) |
There was a problem hiding this comment.
Code changes look good!
I'm only a bit reluctant on adding a prop that is explicitly tied to the visual aspect of the component, which may change over time and make that prop obsolete (i.e. we may decide to always show borders, or to never show borders).
Also, a related question: have we considered "unifying" the look of our toggle controls (aka segmented controls) and always (or never) show borders?
packages/components/src/toggle-group-control/toggle-group-control/styles.ts
Outdated
Show resolved
Hide resolved
My bad, fixed in fa743f7 ✨
I definitely see your point. Since this will require quite a bit more reworking of the code though, I'm hoping we can first play around with it in context when I post a PR to put this into the actual Typography panel. It might be that the "mis-sized" button looks weird, but it also might be that a "mis-sized" focus ring looks weird. That should help us assess how urgent it is to change that detail. |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Yes, I hear you. Would you prefer we name it "isIconGroup" or something like that? @jasmussen Can we pretty much assume that the borderless style will only be for icon button groups? It will also inform how we phrase the style guidance: gutenberg/packages/components/src/toggle-group-control/stories/index.js Lines 142 to 145 in 901814e |
| isIcon = false, | ||
| value, | ||
| children, | ||
| size = 'default', |
There was a problem hiding this comment.
It's weird that TypeScript is allowing this — technically isIcon and size are not defined on neither toggleGroupControlContext nor buttonProps.
I'm not sure if we can pass props that are not part of ToggleGroupControlOptionBase's props via the "context system". If we want to pass "private", internal props, we may need to use useToggleGroupControlContext (and declare these props in the ToggleGroupControlContextProps type)
There was a problem hiding this comment.
It's weird that TypeScript is allowing this
I don't quite understand it either, good catch.
I fixed this up. size is now private to the context system, and isIcon is defined on OptionBase but not the final Option components. Final props tables of the Option components can now be verified in Storybook.
Yes, I'd think that would be a safe bet. |
Given Joen's guidance, I'm now inclined to introduce a new facade component |
|
Exploring on a potential |
|
I don't necessarily think it's the most urgent thing, I trust you can balance it against other tasks on deck, I know there's a lot. |
|
@ciampo The facade component refactor is a bit too big for the scope of this PR, so if it's ok with you I'd like to merge this PR in the current state and move the refactor to a follow-up. I renamed |
There was a problem hiding this comment.
Changes seem to test well on Storybook 🚀
It's not super easy to check for regressions re. size, label and tooltip behavior on the remaining Storybook stories because of the mix of knobs and controls (also, since the stories are still written in JavaScript, we don't get the benefits of the auto-updating controls that we'd otherwise have).
Yeah, I've been putting off the Storybook cleanup for a while 😇 I'll do it before the refactor. |
Part of #36735
Follow-up to #39760 (comment)
What?
__experimentalIsIconGroupvariant to ToggleGroupControl, recommended for use with icon options.Why?
In preparation for migrating TextTransformControl/TextDecorationControl to a ToggleGroupControl implementation so the semantics are correct, and to get a large size variant "for free".
How?
ToggleGroupControlOptionIcon needs to be a different size based on the size variant of the ToggleGroupControl, so we get that via the Context System.
Testing Instructions
npm run storybook:devand see the ToggleGroupControl With Icons story.Screenshots or screencast
Note that the focus state shows the border surrounding the entire ToggleGroupControl, not the individual option. I think this is a good way to maintain consistency within the ToggleGroupControl, and signal that the options are mutually exclusive.
In contrast, if we eventually change the TextDecorationControl implementation to be non-mutually-exclusive (probably semantically a group of checkboxes), the focus ring for that component could surround each individual option.
The is a subtle difference but an important affordance, because the standard focus ring moves with the Tab key, while radio options move with the arrow keys.
CleanShot.2022-08-07.at.00.59.59.mp4