ColorListPicker: refactor to TypeScript #46358
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
added
[Type] Code Quality
HALP!Spot where I'm currently stuck: When typing the
Potential solution: This also feels like the right move to me, because the first argument isn't actually optional - there's a second argument ( I began working on a PR to demo this change, but it had a bit of a domino effect that spills over to other components, so I put a pin in that for now. I'm happy to chase it further if this feels like the right road to travel! |
mirka
reviewed
Dec 7, 2022
There was a problem hiding this comment.
Since this component is not exported publicly and is apparently only used by DuotonePicker, it might be simpler to just move this into the duotone-picker/ folder and type it as part of DuotonePicker. We can always extract it back out when necessary.
Btw I don't mind moving forward with this PR as is (without moving it into DuotonePicker) as an intermediary step. I just wanted to say that it's probably not worth the effort to write new stories/tests/docs for this internal-only component.
Comment on lines
12
to
24
| value?: Array< string >; | ||
| disableCustomColors?: boolean; | ||
| enableAlpha?: boolean; | ||
| onChange: ( newValue: Array< string > ) => void; // TODO: resolve conflict with ColorPalette's onChange prop, which expects an optional value arg. | ||
| }; | ||
|
|
||
| export type ColorOptionProps = Pick< | ||
| ColorListPickerProps, | ||
| 'colors' | 'disableCustomColors' | 'enableAlpha' | ||
| > & { | ||
| label: ColorListPickerProps[ 'labels' ][ number ]; | ||
| value: string; // TODO: can we extract this from ColorListPickerProps['value']? That prop being optional is a little tricky | ||
| onChange: ( newValue: string ) => void; // TODO: resolve conflict with ColorPalette's onChange prop, which expects an optional value arg. |
There was a problem hiding this comment.
I'm not sure if I fully understand the potential solution you described, but I'm assuming it's something along these lines? If so, yes I think it makes sense!
Suggested change
| value?: Array< string >; | |
| disableCustomColors?: boolean; | |
| enableAlpha?: boolean; | |
| onChange: ( newValue: Array< string > ) => void; // TODO: resolve conflict with ColorPalette's onChange prop, which expects an optional value arg. | |
| }; | |
| export type ColorOptionProps = Pick< | |
| ColorListPickerProps, | |
| 'colors' | 'disableCustomColors' | 'enableAlpha' | |
| > & { | |
| label: ColorListPickerProps[ 'labels' ][ number ]; | |
| value: string; // TODO: can we extract this from ColorListPickerProps['value']? That prop being optional is a little tricky | |
| onChange: ( newValue: string ) => void; // TODO: resolve conflict with ColorPalette's onChange prop, which expects an optional value arg. | |
| value?: Array< string | undefined >; | |
| disableCustomColors?: boolean; | |
| enableAlpha?: boolean; | |
| onChange: ( newValue: Array< string | undefined > ) => void; // TODO: resolve conflict with ColorPalette's onChange prop, which expects an optional value arg. | |
| }; | |
| export type ColorOptionProps = Pick< | |
| ColorListPickerProps, | |
| 'colors' | 'disableCustomColors' | 'enableAlpha' | |
| > & { | |
| label: ColorListPickerProps[ 'labels' ][ number ]; | |
| value: string | undefined; // TODO: can we extract this from ColorListPickerProps['value']? That prop being optional is a little tricky | |
| onChange: ( newValue: string | undefined ) => void; // TODO: resolve conflict with ColorPalette's onChange prop, which expects an optional value arg. |
There was a problem hiding this comment.
Thanks @mirka! I'm getting back into this after being out for the holidays. I did consider the | undefined approach, but also (as I tend to do) dove down another rabbit hole at the same time.
The tl;dr of my previously mentioned solution was that the whole reason the undefined is needed is that another componenet (ColorPalette) explicitly passes an undefined value` when de-selecting a color. I think we can use an empty sting instead to both simplify the typing, and prevent a potential edge case bug that TypeScript might not currently see for us, involving an optional parameter that may not be safe to actually omit in all scenarios). More thorough description and a potential change in #46890 😄
If that's not a good direction though, I'm perfectly happy with your proposed solution of adding | undefined as needed!
There was a problem hiding this comment.
Based on the feedback from @jorgefilipecosta on #46890, I've scrapped that approached and gone with string | undefined here.
I've also added some JSDoc descriptions to types. Probably not mission-critical on an internal component like this one but I figured I may as well include them while I was updating things.
Thanks for that, I'll skip those TODOs and we can plan on rolling this into DuotonePicker in a follow up! |
chad1008
force-pushed
the
refactor/ColorListPicker-typescript
branch
from
5e82439 to
f5ae77a
Compare
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 8a6f754. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3962383833
|
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
ColorListPickercomponent to TypeScriptPart of #35744
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
Testing Instructions
Testing Instructions for Keyboard
TBD