Improve Icon component stroybook

[matiasbenedetto]

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

• Clone this PR.
• Compile storybook npm run storybook:dev
• Browse the storybook Icon component page (http://localhost:50240/?path=/story/components-icon--default) and use the component controls.
• Browse the new story page (http://localhost:50240/?path=/story/components-icon--with-an-icon-from-the-library&args=additionalProps.fill:red;size:86;icon:warning)

Screenshots or screencast

Screencast.from.2024-11-22.13-36-57.mp4

[mirka]

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.)

[mirka]

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.

[mirka]

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.

[matiasbenedetto]

Without a value, when the user clicks the control to set a custom value, it defaults to 0, which seems worse than 24.

[mirka]

I think we shouldn't neglect the importance of consistency and user expectations here. Not all components guarantee a static default. To have some controls default to an arbitrary value (which may not even be true) is confusing. If all number controls defaulted to zero, that can be more consistently understood as a meaningless placeholder value.

I think Storybook controls are designed to be used this way, given how certain control types require an actual button click ("Set number", "Set string", "Set boolean" etc.) to reveal the input. That step helps the user understand that the default input value has no meaning. Otherwise, maintainers of a Storybook instance would have to manually specify the default for every single control, which is not sustainable.

[mirka]

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.

[matiasbenedetto]

I understand that Storybook is primarily a tool for users to discover and try components easily. The standard number component is less handy. It defaults to zero without a default set and lets users input negative numbers, which is inconvenient and doesn't reflect the expected use of the component.

[mirka]

And yet that reflects the actual component logic the best, because we don't restrict values, neither at runtime, in docs, nor in TypeScript types. I'm not sure if there's a single number type prop in our library where we do restrict values in any of these ways. Probably because it's usually obvious and goes without saying. It wouldn't be worth the effort to assess and uphold these guardrails for every single number prop.

It's not about what's acceptable for a single prop in a single component, but what consistent patterns we can adopt at scale.

[ciampo]

I agree with Lena here — I understand that the way Storybook controls may feel inconvenient ("I want to set a custom size and it defaults to 0" or "It lets me set a negative size"), but:

• we greatly value consistency across all Storybook examples;
• a consumer of the component would be able to pass "0" or even a negative number to size in the code anyway

[mirka]

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.

[matiasbenedetto]

The motivation is to be able to render any icon of the library and play with the different props passed to that Icon. That's neither possible in the Icon or the icon library story.

[mirka]

Got it. I'll put in some other doc improvements (#67280, #67282) so the story makes sense in context.

Since this is the main use case, I think we can move the story up the file to be the first story before "Fill Color". Let's also add a description and a manual code snippet:

diff --git a/packages/components/src/icon/stories/index.story.tsx b/packages/components/src/icon/stories/index.story.tsx
index 9b0c496f5e..57874f546b 100644
--- a/packages/components/src/icon/stories/index.story.tsx
+++ b/packages/components/src/icon/stories/index.story.tsx
@@ -124,6 +124,10 @@ WithADashicon.args = {
 	icon: 'wordpress',
 };
 
+/**
+ * In most cases, an icon from the `@wordpress/icons` package should be used.
+ * See the Storybook page for a complete [list of icons](https://wordpress.github.io/gutenberg/?path=/story/icons-icon--library).
+ */
 export const WithAnIconFromTheLibrary: StoryFn< IconProps > = ( args ) => {
 	const { additionalProps, ...rest } = args;
 
@@ -148,3 +152,15 @@ WithAnIconFromTheLibrary.argTypes = {
 		control: 'select',
 	},
 };
+
+WithAnIconFromTheLibrary.parameters = {
+	docs: {
+		source: {
+			code: `
+import { wordpress } from '@wordpress/icons';
+
+<Icon icon={ wordpress } />
+		`,
+		},
+	},
+};

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: matiasbenedetto <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: ciampo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.