[Components - ToggleGroupControl]: Update stories to use knobs #34497
Conversation
ntsekouras
added
[Package] Components
ntsekouras
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: 0 B Total Size: 1.04 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hey @ntsekouras, thanks a lot for opening this follow-up PR!
I think it would be great if we could list knobs for all properties of both the ToggleGroupControl and the ToggleGroupControlOption components — and if those knobs all had the same default values that those components would start with (for example, help should default to undefined)
A good example to follow would be the Card component's story, which has knobs for both the Card components and its subcomponents. That story also showcases how values from the Context System are applied to the children sub-components (code here
Also, while reviewing this PR, I realised that the hideLabelFromVision prop is marked as required in the types definition, in contrast with it being defined as not required in the README — I believe we should update the types file and make it optional there, what do you think?
|
Thanks for reviewing @ciampo!
I pushed changes to do this and also added the remaining prop of
done 😄 |
There was a problem hiding this comment.
ToggleGroupControlOption option just has value (which IMO is irrelevant for the story) and label props. Do you think it gives value to be able to just change the label of an option?
It would be probably good to expose the label, if anything for illustrating that it exists as part of the component's APIs
Also, another small nit: regarding the knobs, for the Card story we called them like the props. We should probably apply the same convention here as well, e.g.
hideLabelFromVisionisBlock (rendersToggleGroupControlas a CSS block element)isAdaptiveWidth (renders segments with equal widths)help (Help text)- ...
Finally, while reviewing the story, I came across a potential glitch: the black highlight behind the currently selected item doesn't update when the isAdaptiveWidth is updated
togglegroupcontrol-text-not-centered.mp4
togglegroupcontrol-highlight-not-updating.mp4
Although this can (and probably should) be tackled in a separate PR :)
I think this convention already is in code, no?
I did that now and while I don't have strong opinion on this, I still find this a bit extraneous..
This might be good for a separate PR and probably components team would be a better fit to investigate a bit better. |
There was a problem hiding this comment.
First of all, thank you for working on this follow-up PR, @ntsekouras !
This might be good for a separate PR and probably components team would be a better fit to investigate a bit better.
Absolutely. Although the components team doesn't currently have much capacity (if none at all), I'll try to squeeze it in at some point this week.
Edit: opened #34587 to track this issue separately
At this point, the only "opened" conversations that are left (about knob labels and exposing the label for the ToggleGroupControlOption items) are definitely not blockers.
To give a bit more context to my previous comments — I try to look at this from the point of view of creating/maintaining a component's library, and, with that perspective, my suggestion is aimed at ensuring as much consistency as possible across components' stories (hence the reference to Card, for example), and to make those stories as informative as possible (by, for example, exposing the propName in the knobs).
Having said that, I think that the points that you raised are also totally relevant! Whether you'd like to follow those suggestions or not, feel free to go ahead and merge this PR whenever you're ready.
|
Thanks for the review @ciampo!
I had missed that you meant to change the knob labels like this: Also I had already exposed the label for |
ntsekouras
force-pushed
the
update/toggle-group-control-storybook
branch
from
71a20b3 to
9aaca39
Compare
Description
This PR just updates the stories for
ToggleGroupControlto use knobs for its properties. It's a follow up of: #34017 and specifically this comment.Testing instructions
npm run storybook:devToggleGroupControland change some properties from its knobs