Components: refactor ColorPicker to pass exhaustive-deps
#41294
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: -49 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
I'd been working on some other |
stokesman
added
[Type] Code Quality
|
Awesome, thanks for jumping in @stokesman! I hadn't started in on |
44 tasks
mirka
reviewed
Jun 13, 2022
| @@ -101,7 +101,8 @@ function ColorPicker( { | |||
| if ( onHandleHardwareButtonPress ) { | |||
| onHandleHardwareButtonPress( onButtonPress ); | |||
| } | |||
| }, [] ); | |||
| } ); | |||
| useEffect( bottomSheetInitializer, [ bottomSheetInitializer ] ); | |||
There was a problem hiding this comment.
Hmm... I kind of prefer adding an eslint-disable with a TODO comment here instead of legitimizing the lifecycle-reliant approach of the code. I'm not sure how this component is consumed (or if it has third-party consumers), but it seems to be setting some potential bugs in stone. With an eslint-disable and TODO, we can at least flag that it's problematic. What do you think? (cc @geriux)
There was a problem hiding this comment.
I've gone ahead and changed to the eslint-disable + TODO comment in aa7a41b.
BTW, when I first worked on this I did (for my first time ever) run the iOS emulator to test and maybe see if I could better understand what this effect was for but I couldn't even find a ColorPicker to manipulate 🤷♂️.
mirka
reviewed
Jun 15, 2022
|
I believe the ESLint config has recently been changed around some formatting options, let's rebase this PR to make sure that there are no conflicts or unexpected merge artifacts. |
stokesman
force-pushed
the
update/exhaustive-deps-color-picker
branch
from
602e251
to
f4aa7c4
Compare
|
Having another look at this I'm tempted to do a bit more refactoring for style. Particularly, in Diff to monopolize conditional for legacy handlingdiff --git a/packages/components/src/color-picker/use-deprecated-props.ts b/packages/components/src/color-picker/use-deprecated-props.ts
index cbe57a4e0e..a2cae51d20 100644
--- a/packages/components/src/color-picker/use-deprecated-props.ts
+++ b/packages/components/src/color-picker/use-deprecated-props.ts
@@ -107,30 +107,24 @@ const transformColorStringToLegacyColor = memoize(
export function useDeprecatedProps(
props: LegacyProps | ColorPickerProps
): ColorPickerProps {
- const isUsingLegacy = isLegacyProps( props );
const { onChangeComplete } = props as LegacyProps;
- const { onChange: onChangeProp } = props as ColorPickerProps;
const legacyChangeHandler = useCallback(
( color: string ) => {
onChangeComplete( transformColorStringToLegacyColor( color ) );
},
[ onChangeComplete ]
);
- const onChange = isUsingLegacy ? legacyChangeHandler : onChangeProp;
-
- const { color: colorProp } = props;
- const color = isUsingLegacy
- ? getColorFromLegacyProps( colorProp )
- : ( colorProp as ColorPickerProps[ 'color' ] );
-
- const { disableAlpha } = props as LegacyProps;
- const { enableAlpha: enableAlphaProp } = props as ColorPickerProps;
- const enableAlpha = isUsingLegacy ? ! disableAlpha : enableAlphaProp;
-
+ if ( isLegacyProps( props ) ) {
+ return {
+ color: getColorFromLegacyProps( props.color ),
+ enableAlpha: ! props.disableAlpha,
+ onChange: legacyChangeHandler,
+ };
+ }
return {
- ...( isUsingLegacy ? {} : props ),
- onChange,
- color,
- enableAlpha,
+ ...props,
+ color: props.color as ColorPickerProps[ 'color' ],
+ enableAlpha: ( props as ColorPickerProps ).enableAlpha,
+ onChange: ( props as ColorPickerProps ).onChange,
};
}
|
mirka
reviewed
Jun 16, 2022
|
👋 I see there's still a pending review item but wanted to chime in to say that I'm available to review/test this PR from the native-side when needed. |
ciampo
approved these changes
Jun 29, 2022
There was a problem hiding this comment.
Hey @stokesman , thank you for working on this!
- ✅ Lena's feedback has been addressed
- ✅ Overall code changes LGTM
- ✅ Tested web version of the legacy
ColorPickerin Storybook, works fine - ❓ Native version of the component needs testing
@SiobhyB would you mind taking a look at the native component and potentially give a final approval to this PR? Thank you! 🙏
SiobhyB
approved these changes
Jun 30, 2022
There was a problem hiding this comment.
I tested this on the native side and couldn't spot any regressions in the colour picker. I tested a number of different changes to the background and foreground colour of both a paragraph and a button block. Along the way, I went through the color settings test cases and made sure to experiment with the different options e.g. solid, theme defaults, custom, gradients, etc. All looks good to me! Thanks for including us in this, all :)
|
Thank you @SiobhyB for your review, and thank you @stokesman as always for your contributions, always very appreciated! Could you just add a CHANGELOG entry before merging? |
stokesman
force-pushed
the
update/exhaustive-deps-color-picker
branch
from
a2b1a2b
to
1e6a2aa
Compare
|
Thank you Marco and Siobahn for the reviews and testing. Special thanks to Lena for the thorough advice! |
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?
Updates the
ColorPickercomponent to satisfy theexhaustive-depseslint ruleWhy?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
in
useDeprecatedPropspropsfrom within a callback hookin native component
setColorpassed by props in favor of calling from event handlers.react-hooks/exhaustive-depsrule for an effect hook and left a TODO comment about revisiting it.Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/color-pickerWhat's missing?
A changelog entry but I've left that out for now in case this requires some more time.