ToggleGroupControl: Add size variants #42008
Conversation
| export const medium = css` | ||
| min-height: ${ CONFIG.controlHeight }; | ||
| `; |
There was a problem hiding this comment.
This doesn't look like it's being used anywhere.
| styles.ToggleGroupControl( { size } ), | ||
| isBlock && styles.block, |
There was a problem hiding this comment.
I kind of wanted to refactor this so these styles go in the styles.ts file, but there were some complications. Better done in a separate PR.
| styles.ToggleGroupControl( { size } ), | ||
| isBlock && styles.block, | ||
| 'medium', |
There was a problem hiding this comment.
This was adding a .medium class on the element. Not sure what that was for 🤔 There doesn't seem to be anything in our repo that uses it, at least. I'm inclined to remove it, especially when we're adding more size variants.
There was a problem hiding this comment.
It would be interesting to see if using git-blame we can understand if there was a specific reason for that hardcoded class to exist (just in case the removal causes any visual regression somewhere else in the codebase)
There was a problem hiding this comment.
There are no specific mentions of it in the original PR, but looking at the component in G2 we can surmise that the medium stuff is a copy-paste artifact from the G2 sizing system.
|
Size Change: +25 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Thanks for pointing this out, it looks like I based the designs in 42000 on a slightly outdated component in Figma. I think the unit button there should actually be 36px (the default button size). I'll update my comment. Many apologies!
It's funny you mention that... #41937 :D |
ciampo
added
[Feature] UI Components
There was a problem hiding this comment.
I have a feeling that the "Solid/Gradient" usage should actually be a TabPanel. At the very least, it should have tab panel semantics at the HTML level. At the design level, one could say that the urge to use a variant in this context is a sign that we may be muddling two distinct component functions — ToggleGroupControl is a form input, and is not meant to be used as a tab switcher.
I share the same feelings — ToggleGroupControl has the semantics of a radio button, a TabPanel feels like a better choice.
| background: ${ COLORS.ui.background }; | ||
| border: 1px solid; | ||
| border-color: ${ COLORS.ui.border }; | ||
| border-radius: ${ CONFIG.controlBorderRadius }; | ||
| display: inline-flex; | ||
| min-height: ${ CONFIG.controlHeight }; |
There was a problem hiding this comment.
It looks like the CONFIG holds a few values for controls height:
gutenberg/packages/components/src/utils/config-values.js
Lines 23 to 27 in e4252e4
As we work on standardizing the size variants, we should either:
- update these values and reference them as shared config across our components
- delete them from the shared config to avoid a scenario where some components have their height defined in "locally", and some other components reference a shared config that is out of sync
What do you think?
There was a problem hiding this comment.
delete them from the shared config
Yes 🙈 I think this level of Logic & Order™ is hard to sustain unless they are accounted for at the design stage. Based on our current priorities, we can keep it simple for now.
There was a problem hiding this comment.
Even though I believe that updating the config and using across components would have have been a more clear way to enforce a common height across controls, I'm also ok with deleting the common config and using "local" styles in control components for their heights.
I had a quick look and there aren't too many usages left of controlHeight, so that deletion shouldn't be too hard:
mirka
force-pushed
the
add/toggle-group-control-sizes
branch
from
a87de4e
to
45694d1
Compare
|
I removed the small variant, since it looks like we have a consensus on using |
|
I'm curious if it would be harmful to simply update the default size here, rather than introducing another variant? Having 36px and 40px variants seems a bit untidy? I can't really think of a situation where those 4px will make any meaningful difference. |
There was a problem hiding this comment.
Code changes LGTM and test well as per instructions 🚀
Wondering if it would make sense to add a unit test showing the difference between default and large sizes via toMatchStyleDiffSnapshot (à la Card)
I'm curious if it would be harmful to simply update the default size here, rather than introducing another variant? Having 36px and 40px variants seems a bit untidy? I can't really think of a situation where those 4px will make any meaningful difference.
This is an interesting point — it sounds like we may need to take a moment to reassess our overall strategy when dealing with uniforming control sizes.
| styles.ToggleGroupControl( { size } ), | ||
| isBlock && styles.block, | ||
| 'medium', |
There was a problem hiding this comment.
It would be interesting to see if using git-blame we can understand if there was a specific reason for that hardcoded class to exist (just in case the removal causes any visual regression somewhere else in the codebase)
| background: ${ COLORS.ui.background }; | ||
| border: 1px solid; | ||
| border-color: ${ COLORS.ui.border }; | ||
| border-radius: ${ CONFIG.controlBorderRadius }; | ||
| display: inline-flex; | ||
| min-height: ${ CONFIG.controlHeight }; |
There was a problem hiding this comment.
Even though I believe that updating the config and using across components would have have been a more clear way to enforce a common height across controls, I'm also ok with deleting the common config and using "local" styles in control components for their heights.
I had a quick look and there aren't too many usages left of controlHeight, so that deletion shouldn't be too hard:
Defining some consistent sizing values that can be applied across all controls could be good as a guide. If 40px is the default then other size variants should be meaningfully different, and likely informed by the wider UX. But we should also be aware that additional sizes present opportunity for inconsistency. I am probably too cavalier, but would be in favour of aligning around a single size initially and adapting from there. |
@jameskoster Exactly, this was the Component Squad's concerns in the beginning. After talking to some design folks though, it turned out that the design side wasn't ready to commit to a clear set of size variants across the component library. The main bottlenecks were the older contexts like the block placeholders, or the Post inspector sidebar, which haven't yet been redesigned in the new look & feel. So as devs we kind of had to find a cautious, non-committal middle ground, which was to first get to baseline consistency within the component library (#39397). This work is currently on hold though while we prioritize #41973. Basically the balance we're trying to strike right now is — How can we quickly validate the new 40px designs without:
|
|
Thanks for the additional context. I suppose it doesn't hurt to have both 40 and 36 variants temporarily, if the view is to deprecate one in favor of the other down the road. Seems like a lot of extra work for something that could probably be decided upon from the design perspective though 🤔 |
Part of #41973
What?
Adds large (40px)
and small (32px)size variants to the ToggleGroupControl.Why?
Get the component closer to the design mockups.
Update: The small variant is now removed, in favor of using
TabPanel(#41937).FYI: The small variant appears in this context in the color settings panels:
I'm not sure if 32px is a general size variant we want to commit to across the component library. Has it appeared anywhere else? It will likely appear in the dropdown trigger button for #42000. Otherwise, the "small" button size currently in circulation is 24px. I'm very non-committal about the naming (
__unstable-small).But more fundamentally, I have a feeling that the "Solid/Gradient" usage should actually be a
TabPanel. At the very least, it should have tab panel semantics at the HTML level. At the design level, one could say that the urge to use a variant in this context is a sign that we may be muddling two distinct component functions —ToggleGroupControlis a form input, and is not meant to be used as a tab switcher.@pablohoneyhoney @jameskoster Any thoughts on this? For reference, a similar tab panel pattern can be seen in the main block inserter ("Blocks/Patterns"). We could, for example, switch out the "Solid/Gradient" thing so it uses the
TabPanelcomponent. Another way we could go, if we wanted to converge on theToggleGroupControldesign, is to restyleTabPanelso it looks like a 32pxToggleGroupControl.Testing Instructions
npm run storybook:devToggleGroupControl. There should be asizecontrol to switch the size variants.Screenshots or screencast