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.

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

Merged

Conversation

hideokamoto
Copy link
Contributor

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 definition

Testing Instructions

@codesandbox
Copy link

codesandbox bot commented Nov 20, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

Copy link
Member

@mirka mirka left a 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 */
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

We can remove these @typedefs, now that the descriptions are moved to the types.ts file 👍

Comment on lines 15 to 18
/**
* @param {Props} props
* @return {JSX.Element} Element
*/
Copy link
Member

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 ) {
Copy link
Member

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:

Suggested change
}: DashiconProps ) {
}: WordPressComponentProps< DashiconProps, 'span', false > ) {

@hideokamoto
Copy link
Contributor Author

Thanks, @mirka!
I've updated this PR.

Copy link
Member

@mirka mirka left a 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 @typedefs.
  • Add a changelog.

@@ -7,12 +7,23 @@
*/
/** @typedef {import('react').ComponentPropsWithoutRef<'span'> & OwnProps} Props */
Copy link
Member

Choose a reason for hiding this comment

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

We can remove these @typedefs, now that the descriptions are moved to the types.ts file 👍

@hideokamoto
Copy link
Contributor Author

@mirka
Thank you!
Now I update it.

Copy link
Member

@mirka mirka left a 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! 🚢

@mirka mirka merged commit cf1e1ec into WordPress:trunk Dec 5, 2022
@github-actions github-actions bot added this to the Gutenberg 14.8 milestone Dec 5, 2022
mpkelly pushed a commit to mpkelly/gutenberg that referenced this pull request Dec 7, 2022
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants