Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Icon: Improve icon prop usage docs in Storybook #67280

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Icon: Improve icon prop usage docs in Storybook #67280

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31cb1fc
Icon: Improve `icon` prop usage docs in Storybook
mirka Nov 25, 2024
1e6f347
Fixup
mirka Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 50 additions & 8 deletions packages/components/src/icon/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,26 +47,68 @@ FillColor.args = {
...Default.args,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a control to add a classname anywhere in the stories. Should we have one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts on the className issue are here. I would prefer to align with the standard docgen behavior (i.e. allowed element props are not listed out unless explicitly highlighted).

Although if we get signals that the lack of className documentation is causing confusion, I'm also open to standardizing on "always document className explicitly".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much documentation is rarely an issue, while too little docs has often been an issue. In favor of extenders, I'd personally suggest proactively documenting all props, even if they're general and available for every component. Not something to block this PR though.

Copy link
Contributor

@ciampo ciampo Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyxla , while I agree with you in general, I'm not sure it would be a good idea for our use-case. "All props" means, in many cases, every prop that the underlying HTML element can accept:

  • if we list them all, the docs would become very long and it would be hard for consumers of the component to spot the "main" props;
  • if we don't list them all, how do we draw a line? It feels very arbitrary and potentially even more confusing, since a user may well assume that if a prop is not listed, it can't be specified.

Can we find an alternative solution, maybe adding a paragraph where we explain that the component also accepts props X Y Z (or "every valid attribute and property for the X HTML element")?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All props might be a bit too much, I agree. But useful props folks want to use most often make sense to be documented IMHO.

Can we find an alternative solution, maybe adding a paragraph where we explain that the component also accepts props X Y Z (or "every valid attribute and property for the X HTML element")?

That could work too 👍

};

/**
* When `icon` is a function, it will be passed the `size` prop and any other additional props.
*/
export const WithAFunction = Template.bind( {} );
WithAFunction.args = {
...Default.args,
icon: () => (
<SVG>
<Path d="M5 4v3h5.5v12h3V7H19V4z" />
</SVG>
icon: ( { size }: { size?: number } ) => (
<img
width={ size }
height={ size }
src="https://s.w.org/style/images/about/WordPress-logotype-wmark.png"
alt="WordPress"
/>
),
};
WithAFunction.parameters = {
docs: {
source: {
code: `
<Icon
icon={ ( { size } ) => (
<img
Comment on lines +68 to +71
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid manual snippets as much as possible, but I think it's unavoidable in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's understandable, but on the contrary, those snippets at least look good in the story:

Screenshot 2024-11-26 at 14 09 09

while the code snipped for the default story looks like this:

Screenshot 2024-11-26 at 14 09 22

I wish they were all properly formatted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish they were all properly formatted.

Right 😅 The automatic code snippet engine leaves a lot to be desired. I feel like we might need to start investing more in manual code snippets, since some of them are quite inscrutable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can layer prettier on top of it 😉

width={ size }
height={ size }
src="https://s.w.org/style/images/about/WordPress-logotype-wmark.png"
alt="WordPress"
/>
) }
/>
`,
},
},
};

const MyIconComponent = () => (
<SVG>
const MyIconComponent = ( { size }: { size?: number } ) => (
<SVG width={ size } height={ size }>
<Path d="M5 4v3h5.5v12h3V7H19V4z" />
</SVG>
);

/**
* When `icon` is a component, it will be passed the `size` prop and any other additional props.
*/
export const WithAComponent = Template.bind( {} );
WithAComponent.args = {
...Default.args,
icon: MyIconComponent,
icon: <MyIconComponent />,
};
WithAComponent.parameters = {
docs: {
source: {
code: `
const MyIconComponent = ( { size } ) => (
<SVG width={ size } height={ size }>
<Path d="M5 4v3h5.5v12h3V7H19V4z" />
</SVG>
);

<Icon icon={ <MyIconComponent /> } />
`,
},
},
};

export const WithAnSVG = Template.bind( {} );
Expand All @@ -80,7 +122,7 @@ WithAnSVG.args = {
};

/**
* Although it's preferred to use icons from the `@wordpress/icons` package, Dashicons are still supported,
* Although it's preferred to use icons from the `@wordpress/icons` package, [Dashicons](https://developer.wordpress.org/resource/dashicons/) are still supported,
* as long as you are in a context where the Dashicons stylesheet is loaded. To simulate that here,
* use the Global CSS Injector in the Storybook toolbar at the top and select the "WordPress" preset.
*/
Expand Down
Loading