PaletteEdit: Convert to TypeScript
#47764
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
|
Size Change: +27 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 5296d57. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4344067938
|
mirka
reviewed
Mar 8, 2023
| | PaletteEditGradientsProps; | ||
|
|
||
| type EditingElement = number | null; | ||
| export type SlugPrefix = string; |
There was a problem hiding this comment.
Nit: This SlugPrefix alias kind of feels like overkill to me, especially since TS does not support nominal typing anyway. Could we just write string inline in all the places?
There was a problem hiding this comment.
Good idea, SlugPrefix isn't needed.
Removed it, and inlined string in 327c75c
| @@ -475,10 +571,14 @@ export default function PaletteEdit( { | |||
| { hasElements && ( | |||
| <> | |||
| { isEditing && ( | |||
| <PaletteEditListView | |||
| <PaletteEditListView< typeof elements[ number ] > | |||
There was a problem hiding this comment.
I didn't know we could do this in JSX! 😆
| onChange={ | ||
| onChange as ( | ||
| newElements?: Array< Color | Gradient > | ||
| ) => void |
There was a problem hiding this comment.
I couldn't figure out how to fix this either. Would it be better to just ts-expect-error for now, given that the as here isn't meaningful?
| debounceOnChange( | ||
| // @ts-expect-error | ||
| elements.map( |
There was a problem hiding this comment.
Let's just add some kind of note to signal that it isn't ideal 😄
// @ts-expect-error TODO: Don't know how to resolve| @@ -164,6 +208,7 @@ function Option( { | |||
| <FlexItem> | |||
| <IndicatorStyled | |||
| style={ { background: value, color: 'transparent' } } | |||
| isSelected={ isEditing } | |||
There was a problem hiding this comment.
I might be missing something, but is this supposed to change anything visually? If anything, think it might be better to move this to a separate issue. For now I think it makes sense to change the CircularOptionPicker isSelected prop to be optional.
kienstra
commented
Mar 8, 2023
| @@ -306,9 +324,29 @@ function PaletteEditListView( { | |||
| ); | |||
| } | |||
|
|
|||
| const EMPTY_ARRAY = []; | |||
| const EMPTY_ARRAY: Color[] = []; | |||
|
Hi @mirka, Your suggestions should all be applied now. |
mirka
approved these changes
Mar 13, 2023
| @@ -69,7 +82,6 @@ describe( 'getNameForPosition', () => { | |||
|
|
|||
| describe( 'PaletteEdit', () => { | |||
| const defaultProps = { | |||
| gradients: false, | |||
There was a problem hiding this comment.
Sounds good. For what it's worth, I don't consider this a breaking change, especially since it is just a type check 🙂
…/types.ts Co-authored-by: Lena Morita <lena@jaguchi.com>
…/types.ts Co-authored-by: Lena Morita <lena@jaguchi.com>
|
Hi @mirka, |
|
Cool, feel free to merge whenever you're ready ✨ |
|
Thanks a lot for your commits and review on this, @mirka! |
ciampo
reviewed
Mar 14, 2023
There was a problem hiding this comment.
Thank you both for working on this!
We should keep an eye on PaletteEdit as CustomGradientPicker and GradientPicker get typed, in case we surface any inconsistencies on how PaletteEdit consumes these components (cc @chad1008)
It was great to see some advanced TSX syntax!
Finally, I noticed that we left a couple of @ts-expect-error comments — it would be great to revisit those after CustomGradientPicker and GradientPicker get typed, and understand what's the better way to solve them (maybe through type guards?). Although definitely not a high priority — let's first focus on exhausting the exclude list in tsconfig.json
Similar to how code structure sometimes needs to change drastically when unit tests are added on later, my impression was that this kind of polymorphic component needs to be written in a way that plays nicely with TS. While this component may never become worth such a rewrite, I'm interested to see what kind of better patterns we can establish in new components we write with TS in mind from the very start! |
Absolutely, this is definitely true. In general, going through this migration has showed that such "polymorphic" components are hard to type and probably confusing to consume for downstream developers. In particular, props that change their expected type based on other props is the main patterns that I'd like to avoid. More in general, I believe that adding more typeguards could be the easiest way to retro-fit an existing component, hopefully avoiding the need for a complete rewrite 🤞 |
|
Great points! Ideally, these types are mainly good enough for existing components. Good on you for championing TS and making these much easier to consume. |
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?
Convert the
PaletteEditcomponent to TypeScriptWhy?
As part of an effort to convert
@wordpress/componentsto TypeScriptHow?
Mainly by adding types to the
PaletteEditcomponentTesting Instructions
In the editor
/wp-admin> Edit Site<PaletteEdit>works like before:Storybook
npm run storybook:devPaletteEditDefault storyTesting Instructions for Keyboard
npm run storybook:devScreenshots or screencast
See above