-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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: Auto-generate readme #67282
Icon: Auto-generate readme #67282
Conversation
#### With a Dashicon | ||
Renders a raw icon without any initial styling or wrappers. | ||
|
||
```jsx | ||
import { Icon } from '@wordpress/components'; | ||
import { wordpress } from '@wordpress/icons'; | ||
|
||
const MyIcon = () => <Icon icon="screenoptions" />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These usage examples are moved to the Storybook in #67280.
Instead, the most common use case (using an icon from @wordpress/icons
) is highlighted in the main code snippet.
@@ -25,30 +25,45 @@ export type IconType = | |||
| ( ( props: { size?: number } ) => JSX.Element ) | |||
| JSX.Element; | |||
|
|||
interface BaseProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this unnecessary layer of indirection, and inlined the prop types.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autogenerated READM looks great 👍 🚀
Some stories seem broken, but I'll review #67280 separately, maybe you've addressed them there already.
#### Specifying a className | ||
|
||
```jsx | ||
import { Icon } from '@wordpress/components'; | ||
|
||
const MyIcon = () => <Icon icon="screenoptions" className="example-class" />; | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any mention of className
in #67280, nor here. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. We currently have kind of a mix, where some components pass through all rest props and some only pass through specific props (like className
). In the former case, it is unreasonable to list out all possible props as allowed in TypeScript (id
, aria-*
, etc) and react-docgen-typescript
suppresses them too by default, unless it's required or we add an explicit prop description.
I think almost all of our components pass through at least className
, so it should be one of those things that just work and don't need to be mentioned for every component.
Companion to #67280
What?
Switch the README for
Icon
to an auto-generated one (see also #66035).Why?
To improve accuracy and maintenance cost.
Testing Instructions
npm run docs:components
to verify the changes to the README.