[Components]: Add ToggleGroupControlOptionIcon component
#39760
Conversation
ntsekouras
added
[Type] Enhancement
ntsekouras
changed the title
| * Icon for the option. If `icon` is provided it will be used instead of the `label` | ||
| * and will show a tooltip automatically. | ||
| */ | ||
| icon?: JSX.Element; |
There was a problem hiding this comment.
I'm not sure about the best type here... --cc @ciampo
There was a problem hiding this comment.
JSX.Element is the type I get back from the compiler for components in icons/src/library/*
There was a problem hiding this comment.
This LGTM, but I'd defer to @mirka or @ciampo for their opinions.
I thought an update to the storybook npm packages might fix the args not updating on the canvas, but it didn't for me. Possibly related issue: storybookjs/storybook#16174
| * Icon for the option. If `icon` is provided it will be used instead of the `label` | ||
| * and will show a tooltip automatically. | ||
| */ | ||
| icon?: JSX.Element; |
There was a problem hiding this comment.
JSX.Element is the type I get back from the compiler for components in icons/src/library/*
ntsekouras
force-pushed
the
add/icon-support-toggle-group-control-option
branch
from
5058932 to
d89572e
Compare
|
Size Change: +66 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Super happy to see this getting closer to unification! Pinging @jasmussen for a design review, since it's not entirely clear to me from the Figma mockups what the intended proportions are. (Mostly talking about the horizontal paddings. Should it be narrower so each icon option is a square? It kind of needs to be narrower for it to fit in the 50% sidebar width like in the mockups.)
in storybook the options are not set properly when we switch to different stories
My hunch is that this is due to using both Knobs and Controls in the same stories. I don't think they're designed to coexist. And even on their own, Knobs are pretty buggy when clicking between stories! We'll need to refactor them all to Controls soon.
packages/components/src/toggle-group-control/toggle-group-control-option/styles.ts
Outdated
Show resolved
Hide resolved
|
Thanks for the reviews!
@mirka I saw that padding because I was testing this with changing the current I think when we'll make the transition to other typography controls we might end up with some small differences from the original mockup, and I don't think that's a bad thing. It will be easier to test/iterate there. |
Yup, we should definitely convert our Storybook examples to Controls!
I'm not sure if this is the best strategy for this component's APIs:
We could also evaluate a different approach, which aims at simplifying the API by removing any hidden conventions:
What do folks think? |
|
Thanks for the ping, nice one.
I would caution this a bit, for two reasons. 1: we still need an interface control for a row of toggles, similar to bold/italic/link in the Paragraph toolbar. Here's a couple of examples:
These are more like buttons or individual toggles, than they are items in a group. Furthermore, you can combine both strike-through and underline, so they aren't mutually exclusive, even if in the interface they are at the moment. That's not to say we shouldn't add an icon to the option group, just that it likely can't replace the basic togglegroup.
Yes indeed, it's changed a few times back and forth. Like with inputs, I suspect this is one of the cases where we'd want to support both 36 and 40. I see it at 32px in Global Styles, not sure we need that, but it seems fine to start with 36 and go from there. #38990 and #38577 both use a basic segmented control and the metrics we could start with are these:
Let me know if that was helpful! |
@ciampo we could try that. Personally I think showing both icon and label will make this a bit crowded, but 🤷 . Also if we make label optional, is it okay to allow none of them passed?
@jasmussen these styles are for the component in general, correct? If yes I can open a separate PR for that, as it's not related to this one. |
Yes, the general component. My mistake! |
I'd do icon or label, not both, exactly for the space problems mentioned. If we find a particular need for this combination in the future, we can always revisit, but I wouldn't start with this. |
I agree that it will look crowded — in this scenario, it would be up to the consumer of
It's not a great option, but my reasoning is that a situation where the component doesn't show anything (because neither If the content of the I am basically not a fan of component APIs that rely on conventions like the ones proposed above, e.g "if Other alternative approaches could be:
|
|
I missed this comment when writing my latest reply (thank you @ntsekouras for pointing it out!)
I agree that the approach that I proposed earlier (if both I left some thoughts about other alternative approaches in my last comment, let me know your thoughts! |
|
@jasmussen Talking specifically about the horizontal paddings, should we decrease them to make each icon button a tight square, or keep the current
(No strong opinion either way. It might even make sense to keep them visually distinct like this, so it's easier to grasp the difference between a group that's mutually exclusive and one that's not.) |
@ntsekouras Just FYI, the Components Squad will be coordinating height changes (#39397, though |
|
cc: @ciampo
My vote goes to For example, something like this could be backwards compatible and simple to maintain:
|
If you provide both a label and an icon, can't we use the label just for aria-label? |
Ah, that's a good question. Visually, I'd actually think these work best the same as they are today:
That is — the black buttons are 32x32 inside a borderless 36px tall container. Should we add a borderless version of the segmented control to account for this? |
ntsekouras
force-pushed
the
add/icon-support-toggle-group-control-option
branch
from
176488a to
125fee4
Compare
We could, but I see two drawbacks with this approach:
I'd personally prefer the approach highlighted by @mirka above, which is more straightforward in terms of API design and easier to maintain in the future:
|
There was a problem hiding this comment.
Thank you so much @ntsekouras for applying the feedback!
I've left a few comments, but most of them a very minor requests. We're close to being able to merge this 🎉
packages/components/src/toggle-group-control/toggle-group-control-option-base/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-base/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-base/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-icon/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-icon/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-icon/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/toggle-group-control/toggle-group-control-option-icon/README.md
Outdated
Show resolved
Hide resolved
|
Thanks for the review and the help here @ciampo! Feedback addressed 😄 |
ntsekouras
force-pushed
the
add/icon-support-toggle-group-control-option
branch
from
2d00190 to
8c69689
Compare
ntsekouras
force-pushed
the
add/icon-support-toggle-group-control-option
branch
from
8c69689 to
05e1573
Compare
ntsekouras
changed the title
ToggleGroupControlOptionIcon component
There was a problem hiding this comment.
Thank you for your patience when working on this PR, @ntsekouras !
Changes LGTM 🚀 (not sure if we want to get a final 👍 from @jasmussen and/or @mirka )
packages/components/src/toggle-group-control/toggle-group-control-option-icon/README.md
Outdated
Show resolved
Hide resolved
|
I'll trust you all on the inner mechanisms here! What's important on the visual side is that the letter case/text transform remain visually as they appear here. and aren't suddenly bordered or something. |
What?
Part of: #36230
Needed for: #36735
This PR adds
iconsupport toToggleGroupControlOptioncomponent (child of ToggleGroupControl).Why?
This will be needed for implementing other controls like
textTransformortextDecorationto useToggleGroupControl.How?
This implementation makes the use of icon and label mutually exclusive. What this means is that if we provide an
iconit will be used instead of thelabeland a tooltip will be shown automatically.Testing Instructions
Notes
I've noticed that in storybook the options are not set properly when we switch to different stories. It's not related to this PR and it should be handled separately.
Screen.Recording.2022-03-25.at.2.36.20.PM.mov
Screenshots or screencast
Screen.Recording.2022-03-25.at.2.22.27.PM.mov