Improve Icon component stroybook #67242
Conversation
matiasbenedetto
added
[Type] Enhancement
| title: 'Components/Icon', | ||
| component: Icon, | ||
| parameters: { | ||
| controls: { expanded: true }, | ||
| docs: { canvas: { sourceState: 'shown' } }, | ||
| }, | ||
| args: { | ||
| additionalProps: {}, |
There was a problem hiding this comment.
I'm afraid we can't really do this because it makes it look like additionalProps is an actual prop. The props table is not just for tweaking Storybook controls, but actually needs to function as props documentation. (Not saying this is true for every Storybook instance, but in our case we've designated it as the canonical reference for props documentation.)
There was a problem hiding this comment.
How about we add a control for just fill maybe? It's guaranteed to work as part of the TypeScript types of the component, it's a common use case, and we even feature it in one of the stories.
| title: 'Components/Icon', | ||
| component: Icon, | ||
| parameters: { | ||
| controls: { expanded: true }, | ||
| docs: { canvas: { sourceState: 'shown' } }, | ||
| }, | ||
| args: { | ||
| additionalProps: {}, | ||
| size: 24, |
There was a problem hiding this comment.
We shouldn't hardcode this because it overrides the default logic (20 when icon is a string). Also in general we try to avoid hardcoding default values in the Storybook, and instead just let it render the default as defined by the component's own code.
| size: { | ||
| control: { | ||
| type: 'range', | ||
| min: 1, | ||
| max: 200, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Similar to the discussions we've been having about range inputs in general (#60633), but range inputs imply that a min/max exists, which is untrue in this case. A standard number input is more in line with all the other props controls in our Storybook.
| @@ -99,3 +123,28 @@ WithADashicon.args = { | |||
| ...Default.args, | |||
| icon: 'wordpress', | |||
| }; | |||
|
|
|||
| export const WithAnIconFromTheLibrary: StoryFn< IconProps > = ( args ) => { | |||
There was a problem hiding this comment.
What was the motivation for adding this? I think we can improve it based on that. For example, I can think of two motivators:
- I want to show that we can use icons from the library. In this case, it would be more useful to have a description for the story, and an actual code snippet showing the imports.
- I want to show what icons there are in the library. In this case, it would be more useful to send people to the icon library story, which shows the entire set with a search field.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
What?
Improve the Icon component storybook.
Why?
This is to make it easier to test additional props on icons.
To make more accessible to test library icons.
How?
With additions to the Icon storybook Icon component template.
By adding a new story to select a icons library icon from a select.
Testing Instructions
npm run storybook:devScreenshots or screencast
Screencast.from.2024-11-22.13-36-57.mp4