CircularOptionPicker: refactor to TypeScript #47937
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
chad1008
force-pushed
the
refactor/CircularOptionPicker-typescript
branch
2 times, most recently
from
18b061a
to
c04ba09
Compare
chad1008
commented
Feb 9, 2023
chad1008
added
[Type] Code Quality
chad1008
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +402 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Feb 13, 2023
There was a problem hiding this comment.
Nicely done!
✅ No runtime changes
✅ Types look good to me
✅ Did a quick smoke test in GradientPicker's Storybook example
Apart from the inline comments, would you be able to add a simple Storybook example? Nothing too complex, just a "standard" default example to show the auto-generated docs and test the component in isolation.
|
Thanks for the review @ciampo! I've updated the README with the subcomponents, and I've also added a story. While I was there I added stories to demonstrate the two subcomponents, in part because I wanted to experiment with |
|
Shoot, I almost forgot: quick question about formatting the code examples in Storybook: I've used an array of options and then Is there a trick to force some indentation there, or would we be better off just hardcoding those |
chad1008
commented
Feb 14, 2023
packages/components/src/circular-option-picker/stories/index.tsx
Outdated
Show resolved
Hide resolved
chad1008
commented
Feb 14, 2023
ciampo
reviewed
Feb 15, 2023
There was a problem hiding this comment.
Thank you for working on the README and the Storybook example.
I've left inline comments, but overall changes are looking great 🎉
...this leads to the options prop in the code examples not being formatted nicely.
Is there a trick to force some indentation there, or would we be better off just hardcoding those Options subcomponents so we can just explicitly format/indent as we see fit?
I'm not sure I understand what you mean — how would I be able to see the issues with the formatting?
packages/components/src/circular-option-picker/stories/index.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Feb 15, 2023
packages/components/src/circular-option-picker/stories/index.tsx
Outdated
Show resolved
Hide resolved
Sorry, I wasn't clear in my description. On the Docs tab in Storybook, the code example was rendering with the I think I'm caught up on existing feedback. I'll address the CHANGELOG conflict as a final step when the other ducks are all in a row. Thank you again for all of the detailed feedback! |
ciampo
approved these changes
Feb 16, 2023
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
chad1008
force-pushed
the
refactor/CircularOptionPicker-typescript
branch
from
495a911
to
6ca7c5f
Compare
|
Thank you for all of the thoughtful help and feedback @ciampo! |
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?
Refactor
CircularOptionPickercomponent to TypeScriptPart of #35744
Co-authored with @ciampo
Why?
The refactor to TypeScript has many benefits (auto-generated docs, static linting and error prevention, better IDE experience). See #35744 for more details
How?
Followed the steps in the TypeScript migration guide
I've also added a README for the component.
Testing Instructions
ColorPalette,DuotonePicker, andGradientPickerstill work as expected.