-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Dashicon: refactor to TypeScript #45924
Dashicon: refactor to TypeScript #45924
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
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.
Thanks for contributing! 🙌
I left a few comments around repo conventions, but it's looking good. Also if you could add a quick changelog entry before merging, that would be great.
@@ -7,12 +7,23 @@ | |||
*/ | |||
/** @typedef {import('react').ComponentPropsWithoutRef<'span'> & OwnProps} Props */ |
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 JSDocs can be removed as well, although we like to move the prop descriptions (e.g. "Icon name", "Size of the icon") to inline JSDocs in the types.ts
file (example). This way we can see the descriptions in IntelliSense, docgens, etc.
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.
We can remove these @typedef
s, now that the descriptions are moved to the types.ts file 👍
/** | ||
* @param {Props} props | ||
* @return {JSX.Element} Element | ||
*/ |
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.
Looks like we can remove these JSDocs.
size = 20, | ||
style = {}, | ||
...extraProps | ||
}: DashiconProps ) { |
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.
You don't have to go through the entire TS migration guide because Dashicon component is really simple and doesn't have any stories/tests. But it might help to see item vi
of bullet point 7, where it describes how we use the WordPressComponentProps
utility type for cases like this, instead of ComponentPropsWithoutRef
.
You can see some usage examples in other components, for example:
}: WordPressComponentProps< SearchControlProps, 'input', false >, |
So here that would look like:
}: DashiconProps ) { | |
}: WordPressComponentProps< DashiconProps, 'span', false > ) { |
Thanks, @mirka! |
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.
Thanks, just two more things before merge:
- Remove the remaining
@typedef
s. - Add a changelog.
@@ -7,12 +7,23 @@ | |||
*/ | |||
/** @typedef {import('react').ComponentPropsWithoutRef<'span'> & OwnProps} Props */ |
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.
We can remove these @typedef
s, now that the descriptions are moved to the types.ts file 👍
@mirka |
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.
Nice, thanks again for contributing! 🚢
* Dashicon: refactor to TypeScript * chore: fix the type definision on the Dashicon component * chore: remove unuse typedef on the Dashicon component * chore: update the changelog on packages/components
What?
Refactor the Dashicon component to TypeScript
Ref: #35744
Why?
To contribute to this issue: #35744
How?
Replace the
Dashicon
component file with tsx, and put the type definitionTesting Instructions