-
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
Refactor Toolbar component to TypeScript #47087
Refactor Toolbar component to TypeScript #47087
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @mike-day! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Hey @mike-day , thank you so much for starting the work on this! I will try to come back with a proper review later, but for now I just wanted to say that there will be some conflicts with PR #47117 — I would recommend you wait until that's merged (hopefully within 1-2 days) and then rebase your PR on top of latest |
Thanks for following up about this so quickly, @ciampo! I see that #47117 is merged, and I should be able to rebase and resolve conflicts within the next few days. I will mark this as ready for review and ping you again soon! |
…factor/toolbar-component-to-typescript
…factor/toolbar-component-to-typescript
Regardless of a few failing mobile tests, I believe this PR is at a decent starting point if you want to take a look, @ciampo. Thanks! |
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! 🙌 Good call to keep the scope of this PR to a manageable size.
I just had a few comments, but on the whole this looks clean and pretty close to merging!
packages/components/tsconfig.json
Outdated
@@ -60,7 +60,6 @@ | |||
"src/notice", | |||
"src/palette-edit", | |||
"src/panel", | |||
"src/toolbar", |
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.
Nitty, but should we add the toolbar subfolders that are not typed yet here, instead of @ts-ignore
at the top of the files?
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.
If they're not going to be typed in the next week or so, I think I prefer the ts-nocheck
s so we can clear out this exclude list and ship the package types 🙈
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.
Ah, I see! But wouldn't it be weird to ship partial types for Toolbar
?
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.
Fwiw, I am starting work on toolbar-button
today with a general plan to run through the remaining toolbar subfolders this month 💻
Co-authored-by: Lena Morita <[email protected]>
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 great, thank you! We can merge once the changelog is tweaked 🚀
Part of #35744
What?
Convert the
Toolbar
component to TypeScript.Why?
As a contribution to the ongoing effort to convert
@WordPress/components
to TypeScript.How?
By adding types to
Toolbar
,@ts-nocheck
where needed for relatedToolbar
components, and refactoring of stories.Testing Instructions
Image
)