FontSizePicker: Refactor stories to use Controls #38727
Conversation
mirka
added
[Package] Components
|
Size Change: +1.09 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you @mirka , this is a great refactor!
Not directly a consequence of this PR, but I got confused when toggling the withReset prop — even when true, the reset button is only shown when enabling a custom size while not showing a slider:
- the docs for the prop are not very clear to me: "If
true, a reset button will be displayed alongside the predefined and custom font size fields.". Should we update them? - We should probably add a similar sentence as a note to the Storybook toggle, to avoid further confusion
Also looking forward to having @ntsekouras and @aaronrobertshaw take a look at this too.
There was a problem hiding this comment.
Thanks for the PR @mirka !
the docs for the prop are not very clear to me: "If true, a reset button will be displayed alongside the predefined and custom font size fields.". Should we update them?
Yes, we should do that! It through me off at first and had to carefully read the code again to see what is expected and what's not 😄
In general I like the direction of this, but I'm wondering if we should revisit what stories we should show now. Previously with the knobs, not all properties were editable and were there to show case some combinations - thus @ciampo 's confusion (and mine) with the reset or disableCustomFontSizes, etc..
I think we can reduce the stories shown here and combine them by changing some names. For example WithoutCustomSizes story is identical with the default except on property, but by being able to edit the props it starts being confusing.
Maybe it makes sense to always show the fewer/more font sizes? 🤔
Alternatively we could disable some controls to mimic the previous behavior.
What do you think?
There was a problem hiding this comment.
Thanks for getting this refactor started @mirka 👍
All the font size picker examples work as advertised for me.
@ntsekouras raises some good points though. I think for a lot of components we could combine examples however for others it makes sense to demonstrate a certain configuration and behaviour. Allowing all props to be edited in these scenarios definitely adds confusion and doesn't seem right.
I'd also like to see some examples being clearer in what they are supposed to demonstrate. One example of this could be "With Complex CSS Values". I'm not sure someone looking at these for the first time would pick that the difference between this example and "With Units" is the labelling of the segmented control. Could we communicate this better within the example even if just with some help text below it explaining/highlighting the demonstrated behaviour?
These aren't issues specifically with this PR of course however it would be good to form a consensus plan here so it can be followed for other components.
These can be removed once the component is TS converted
These can be removed once the component is TS converted
This would be more convenient https://github.com/izhan/storybook-description-loader
|
Thanks for all the feedback, you all raise very good points — the main sentiment being that it's hard to understand what the props do or what the stories are trying to demonstrate. I went back and did some thinking on how we should be approaching Storybook, and the ideas are discussed in #38842. Based on that, I think the direction we’ll be pursuing is to make Docs view (instead of Canvas view) the primary focus of our Storybook. Once a component is written in TypeScript, the docgen will not only extract all the prop data into a table, but also auto-generate the Controls for every prop. That is super convenient, but also that alone would perhaps make some stories overwhelming to understand. However, in Docs view we can show descriptions and actual code snippets for each story, which is actually more useful to most devs than having just a curated set of Controls to tinker with. The Canvas view would instead be more of a place to do advanced tinkering, with all the Controls present. While
How does it feel now? CleanShot.2022-02-19.at.03.58.59.mp4 |
| WithCustomSizesDisabled.parameters = { | ||
| docs: { | ||
| description: { | ||
| story: | ||
| 'When disabled, the user will only be able to pick one of the predefined sizes passed in `fontSizes`.', | ||
| }, | ||
| ] ); | ||
| return ( | ||
| <FontSizePickerWithState | ||
| fontSizes={ fontSizes } | ||
| disableCustomFontSizes | ||
| /> | ||
| ); | ||
| }, |
There was a problem hiding this comment.
This is kind of verbose for just wanting to set a story description, so I'm thinking of using a webpack loader that lets you write the description as a normal JSDoc comment.
There was a problem hiding this comment.
Do you think there's any chance of breaking our story description in case Storybook updates their APIs? The webpack loader that you linked to hasn't been updated in more than a year
Anyway, we can definitely experiment with it in a separate PR! (although definitely not high priority as it's just a minor improvement)
There was a problem hiding this comment.
The loader code is pretty trivial and there's a reasonable chance that it will become a built-in feature, so I'm not too concerned about that. I might even try keeping the loader code local, rather than as an external dep.
There was a problem hiding this comment.
I think the direction we’ll be pursuing is to make Docs view (instead of Canvas view) the primary focus of our Storybook.
...
The Canvas view would instead be more of a place to do advanced tinkering, with all the Controls present.
I also think that this is the right direction to take
Once a component is written in TypeScript, the docgen will not only extract all the prop data into a table, but also auto-generate the Controls for every prop. That is super convenient, but also that alone would perhaps make some stories overwhelming to understand.
Those considerations are all correct, and (as discussed in #38842) I think we can mitigate the potentially overwhelming amount of props / controls in a few ways (e.g. sorting, grouping..)
While FontSizePicker is not in TypeScript yet, I made some changes with that eventuality in mind:
- Make code snippets visible in Docs view, and checked that they were understandable.
- Manually added descriptions to confusing props. (This will be automatic when the component is converted to TS)
- Added descriptions to some stories to clarify what they were trying to demonstrate.
How does it feel now?
I think it's much better already! Although it's probably wiser to hear at least @ntsekouras 's opinion too before merging (I know Aaron may be AFK for a while)
Also, is it expected that the header hint in the "With Units" Story only has the unit (px) in the toggle-group version of the picker, but has the quantity + the unit (12px) in the dropdown-version?
Finally — should we mention the stories rework (and the couple of bug fixes) in a CHANGELOG entry?
| WithCustomSizesDisabled.parameters = { | ||
| docs: { | ||
| description: { | ||
| story: | ||
| 'When disabled, the user will only be able to pick one of the predefined sizes passed in `fontSizes`.', | ||
| }, | ||
| ] ); | ||
| return ( | ||
| <FontSizePickerWithState | ||
| fontSizes={ fontSizes } | ||
| disableCustomFontSizes | ||
| /> | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Do you think there's any chance of breaking our story description in case Storybook updates their APIs? The webpack loader that you linked to hasn't been updated in more than a year
Anyway, we can definitely experiment with it in a separate PR! (although definitely not high priority as it's just a minor improvement)
Yes. The same information is displayed in different places (hint, labels, etc..). |
There was a problem hiding this comment.
Thanks @mirka! That's so much better now with all the descriptions and hints! Great work 🚀
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Part of #35665
In preparation for #38720
Description
Refactors the
FontSizePickerstories to use Controls instead of Knobs, and reworks some of the stories for clarity and correctness.Reworked stories
fallbackFontSizeprop only kicks in wheninitialValueis0orundefined, but this was not set correctly so it wasn't showcasing the fallback behavior.initialValueof8instead of8px, thus showcasing a custom value by default.I tried not to drop any of the intent captured in the original stories — let me know if I missed something.
Testing Instructions
npm run storybook:devand see the stories forFontSizePicker.Pro tip: Knobs can be kind of buggy when you click through multiple stories. Click the "Reset" button in the bottom right corner if they get stuck.
Types of changes
Storybook only.
Checklist:
*.native.jsfiles for terms that need renaming or removal).