CheckboxControl: Convert component to TypeScript #40915
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
walbo
added
[Type] Enhancement
added 2 commits
May 8, 2022 19:38
|
Size Change: +781 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I won't likely be able to review this PR myself, but I've tagged a few other folks who have experience with previous refactors and should be able to help 💯 |
noisysocks
reviewed
May 11, 2022
|
Nice one! 🌈 Storybook looks good when I test locally, aside from the example not appearing (#40915 (comment)), and I don't see any obvious regressions in the block editor. Would be nice to add a few simple |
aristath
changed the title
Can do a follow-up PR adding some tests 👍 |
noisysocks
approved these changes
May 12, 2022
aaronrobertshaw
requested changes
May 12, 2022
There was a problem hiding this comment.
Thanks for tackling this one @walbo 👍
This one is looking pretty good. I have a few minor tweaks and questions I think it would be good to address before merging.
✅ Readme prop descriptions match types
✅ Storybook examples are all functioning well
❓ I don't think we really need additional simple examples, e.g. WithLabel, if we just have default arg values. The conversion guidelines suggest the default story should be an interactive playground for the component. Wouldn't that suffice?
vii. Use the template for the Default story, which will serve as an interactive doc playground.
❓ A ref prop is appearing in the controls and looks to come from WordPressComponentProps.
Not sure if this one makes sense here and we need something like WordPressComponentPropsWithoutRef.
The ref prop is removed from the story and everything appears to function for me if I omit ref from the CheckboxControl props type. Perhaps @mirka could point to the proper approach here?
❓ The deprecated heading prop has a controls table entry. We should probably remove/disable this to discourage its use further.
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Yes, I agree. Updated the default story to have the same default args as in the example. |
aaronrobertshaw
approved these changes
May 13, 2022
There was a problem hiding this comment.
Thanks for the speedy turnaround on this @walbo 🚀
✅ Storybook examples are all functioning well
✅ Simplified examples look good
✅ Readme prop descriptions match types
✅ Deprecated prop no longer advertised in controls or docs
✅ All other review suggestions resolved
Nice work! LGTM 🚢
|
Thanks for the review and feedback @noisysocks and @aaronrobertshaw. Much appreciated 😃 |
|
Thank you all for collaborating on this one!
@walbo , feel to ping when the follow-up PR is ready 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Converts the
CheckboxControlcomponent to TypeScript.Why?
Part of the @wordpress/components's TypeScript migration (#35744).
How?
Refactors the current
CheckboxControlto TypeScript.Testing Instructions
CheckboxControlcontinues to function as expected