DuotonePicker, DuotoneSwatch: Convert to TypeScript #49060
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
mirka
commented
Mar 14, 2023
| /** | ||
| * An array of colors for the duotone effect. | ||
| */ | ||
| value?: string[] | 'unset'; |
There was a problem hiding this comment.
I considered whether it was worth doing a tuple ([ string, string ]) here instead of string[], but decided against it for two reasons:
- The upstream
CustomGradientPickeracceptsstring[], so we'll need to do some added type massaging there. - I noticed when typing the stories that consumers may be forced to do
[ '#foo', '#bar' ] as constto make the type checks pass, which is non-obvious.
Does that sound reasonable?
There was a problem hiding this comment.
A tuple would be definitely better in representing the actual type that the component is expecting — without it, TS wouldn't be able to detect a malformed value (ie. [], [ '#fff' ] etc).
But the points that you make are valid, and therefore I'd be ok with typing it as string[], at least initially. We can always narrow the type (or put more runtime checks in place) later as we see fit
There was a problem hiding this comment.
As you pointed out recently, the new const type improvements may come handy for this scenario!
Comment on lines
+48
to
54
| [ | ||
| { brightness: 1, color: '' }, | ||
| { brightness: 0, color: '' }, | ||
| ] | ||
| ) | ||
| .map( ( { color } ) => color ); | ||
| } |
There was a problem hiding this comment.
An interesting example of how some clever functional code may not play nice with TS 😆 The initial values ([ { brightness: 1 }, { brightness: 0 } ]) will never be remaining at the point of the final .map(), but TS cannot know that.
I think this is probably the least annoying/invasive way around it.
There was a problem hiding this comment.
I think this is probably the least annoying/invasive way around it.
Agreed!
Although annoying at times, I'm glad that TS is able to pick up these edge cases. The fact that the author of these lines of code was clever in writing the original implementation doesn't necessarily mean that a malformed color palette (e.g with out of scale brightness values) or another developer making amends to the algorithm could introduce a bug later in time. TS checks are great in avoiding such events
There was a problem hiding this comment.
Though for this particular function I would rely on some unit tests more than TS 🫣
There was a problem hiding this comment.
That's true! I guess in my mind I implicitly considered TS static linting as a the first test that runs against the code.
chad1008
reviewed
Mar 14, 2023
packages/components/src/duotone-picker/color-list-picker/style.scss
Outdated
Show resolved
Hide resolved
|
Size Change: +242 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Mar 15, 2023
| @@ -125,6 +159,7 @@ function DuotonePicker( { | |||
| newColors.length >= 2 | |||
| ? newColors | |||
| : undefined; | |||
| // @ts-expect-error TODO: Investigate if this is actually a problem | |||
There was a problem hiding this comment.
There's definitely a type mismatch at some point here. ColorListPicker declares that both its value and its onChange deal with Array< string | undefined >, while DuotonePicker doesn't accept undefined in its types.
My gut instinct would be to:
- understand what actually happened in
DuotonePickerandColorListPickerwhen the value wasundefined - maintain the same behaviour, while adding more explicit checks (ie. small runtime changes) if necessary (e.g adding a typeguard in
ColorListPickeragainstundefinedvalues?)
There was a problem hiding this comment.
Alright, @chad1008 and I took a deeper look at this, and came to the conclusion that in order to fix the TS mismatch properly we'd need to rewrite the component in such a way that enforces value and onChange to deal with a 2 item array (which kind of relates to this other convo ).
A quicker way to address the TS error for this PR, while introducing the least amount of runtime changes possible, could be:
- (Optional) Slightly tweak
ColorListPickertypes
Like this
diff --git a/packages/components/src/duotone-picker/color-list-picker/index.tsx b/packages/components/src/duotone-picker/color-list-picker/index.tsx
index 8a65382482..cff79027d0 100644
--- a/packages/components/src/duotone-picker/color-list-picker/index.tsx
+++ b/packages/components/src/duotone-picker/color-list-picker/index.tsx
@@ -75,9 +75,11 @@ function ColorListPicker( {
disableCustomColors={ disableCustomColors }
enableAlpha={ enableAlpha }
onChange={ ( newColor ) => {
- const newColors = value.slice();
- newColors[ index ] = newColor;
- onChange( newColors );
+ onChange( [
+ ...value.slice( 0, index ),
+ newColor,
+ ...value.slice( index + 1 ),
+ ] );
} }
/>
) ) }
diff --git a/packages/components/src/duotone-picker/color-list-picker/types.ts b/packages/components/src/duotone-picker/color-list-picker/types.ts
index becfc405b2..73c1900235 100644
--- a/packages/components/src/duotone-picker/color-list-picker/types.ts
+++ b/packages/components/src/duotone-picker/color-list-picker/types.ts
@@ -21,7 +21,7 @@ export type ColorListPickerProps = {
/**
* An array containing the currently selected colors.
*/
- value?: Array< string | undefined >;
+ value?: Array< string >;
/**
* Controls whether the custom color picker is displayed.
*/
- Cast the
newColorsarray asstring[]. This is not a 100% correct cast, since in theorynewColorscould still haveundefineditems at index 2 and up, but we kind of know that in practice that's not supposed to happen.
Like this
diff --git a/packages/components/src/duotone-picker/duotone-picker.tsx b/packages/components/src/duotone-picker/duotone-picker.tsx
index 7d854242e5..4f909b01fe 100644
--- a/packages/components/src/duotone-picker/duotone-picker.tsx
+++ b/packages/components/src/duotone-picker/duotone-picker.tsx
@@ -157,9 +157,8 @@ function DuotonePicker( {
}
const newValue =
newColors.length >= 2
- ? newColors
+ ? ( newColors as string[] )
: undefined;
- // @ts-expect-error TODO: Investigate if this is actually a problem
onChange( newValue );
} }
/>
There was a problem hiding this comment.
Thanks for looking into it, Marco + Chad!
I'm not quite convinced that the string[] cast is any better than a ts-expect-error — if anything it can imply a false sense of safety. Do you prefer this cast rather than leaving the error for proper fixing later? If not, I can update the TODO comment so it links to this PR thread 😄
There was a problem hiding this comment.
You make a good point — we can leave the @ts-expect-error instead of the typecast to avoid the false sense of safety (plus, we will get a TS error if we ever fix those types!).
What do you think about the tweaks suggested in the first point above?
There was a problem hiding this comment.
Works for me!
Last thing I'd suggest is to improve the comment associated to the @ts-expect-error, explaining that it caused by the fact that typescript doesn't know that both newColors and value are in fact supposed to be a 2 item tuple, instead of a string[]
Comment on lines
+48
to
54
| [ | ||
| { brightness: 1, color: '' }, | ||
| { brightness: 0, color: '' }, | ||
| ] | ||
| ) | ||
| .map( ( { color } ) => color ); | ||
| } |
There was a problem hiding this comment.
I think this is probably the least annoying/invasive way around it.
Agreed!
Although annoying at times, I'm glad that TS is able to pick up these edge cases. The fact that the author of these lines of code was clever in writing the original implementation doesn't necessarily mean that a malformed color palette (e.g with out of scale brightness values) or another developer making amends to the algorithm could introduce a bug later in time. TS checks are great in avoiding such events
| /** | ||
| * An array of colors for the duotone effect. | ||
| */ | ||
| value?: string[] | 'unset'; |
There was a problem hiding this comment.
A tuple would be definitely better in representing the actual type that the component is expecting — without it, TS wouldn't be able to detect a malformed value (ie. [], [ '#fff' ] etc).
But the points that you make are valid, and therefore I'd be ok with typing it as string[], at least initially. We can always narrow the type (or put more runtime checks in place) later as we see fit
ciampo
approved these changes
Mar 21, 2023
There was a problem hiding this comment.
🚀
While testing the component in storybook, I noticed a couple of glitches with the swatch tooltip that we should iron out as a follow-up:
- When the duotone "gradient bar" has a set value, the tooltip renders below the bar
- When the gradient bar doesn't have a value, the tooltip can be seen, but its text is transparent instead of being white — ie. the color of the text is whatever color is the UI behind the tooltip (notice how the "U" in "Unset" is partially white, and then grey)
| @@ -125,6 +159,7 @@ function DuotonePicker( { | |||
| newColors.length >= 2 | |||
| ? newColors | |||
| : undefined; | |||
| // @ts-expect-error TODO: Investigate if this is actually a problem | |||
There was a problem hiding this comment.
Works for me!
Last thing I'd suggest is to improve the comment associated to the @ts-expect-error, explaining that it caused by the fact that typescript doesn't know that both newColors and value are in fact supposed to be a 2 item tuple, instead of a string[]
packages/components/src/duotone-picker/color-list-picker/types.ts
Outdated
Show resolved
Hide resolved
| onChange: ( value: DuotonePickerProps[ 'value' ] | undefined ) => void; | ||
| }; | ||
|
|
||
| type Color = { |
There was a problem hiding this comment.
(Not required for this PR, so feel free to ignore)
As we work on color-related components, it would be great to take a wider look at these components' types, and see if we can better highlight dependencies and/or find inconsisntencies.
For example, the Color type here (and the type of the colors prop for the ColorListPicker component) could be derived from the ColorObject type from ColorPalette
|
Extracted the Tooltip rendering issue 👉 #49225 Thank you for the great reviews! |
|
Flaky tests detected in 8142875. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4478442330
|
ryanwelcher
added
the
[Type] Code Quality
3 tasks
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.
Part of #35744
What?
DuotonePickerandDuotoneSwatchto TypeScript.ColorListPickerinto theduotone-pickerfolder (it's not exported publicly and only used here).Testing Instructions
npm run storybook:devand see stories for DuotonePicker and DuotoneSwatch.