Components: refactor ColorPalette to disable exhaustive-deps check for now
#41253
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
44 tasks
chad1008
changed the title
isSelectedCustom() in useCallback to ensure re…ColorPalette to pass exhaustive-deps
|
Size Change: 0 B Total Size: 1.24 MB ℹ️ View Unchanged
|
|
Added @geriux too in order to have a reviewer with more experience on React native (in case) |
ciampo
reviewed
May 23, 2022
chad1008
force-pushed
the
enhance/ColorPalette-exhuastive-deps
branch
from
cf25a13
to
7a5f206
Compare
ciampo
reviewed
May 23, 2022
geriux
added
the
Mobile App - i.e. Android or iOS
ciampo
added
[Type] Code Quality
Good catch! Weird that the linter didn't catch it though |
chad1008
force-pushed
the
enhance/ColorPalette-exhuastive-deps
branch
from
9564af3
to
b37de51
Compare
|
@geriux would you be able to take another look? Thank you! |
|
Thanks for updating the code! I've tested again and I found another issue with the We can keep the This is the issue we have with the ColorPalette.mov |
|
Thank you @geriux for taking another look!
This is usually a code smell, hinting that the code could be rewritten differently. I looked into the code a bit more, and noticed that the If we have to disable What do folks think? |
Definitely, adding more things to the dependencies list might cause other side effects not expected when this component was created. And I agree that it would need to be refactored a so it works correctly.
I like that suggestion, until we have some time to check that component and make the necessary code changes. I'm actually happy we are planning to add the |
|
Sounds like trying to get this one safely passing |
chad1008
force-pushed
the
enhance/ColorPalette-exhuastive-deps
branch
from
b37de51
to
da92d8f
Compare
chad1008
changed the title
ColorPalette to pass exhaustive-deps ColorPalette to disable exhaustive-deps check for now
+1 to that! Thank you! |
chad1008
force-pushed
the
enhance/ColorPalette-exhuastive-deps
branch
from
da92d8f
to
87f6779
Compare
youknowriad
pushed a commit
that referenced
this pull request
Jun 3, 2022
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
ColorPalettecomponent to satisfy theexhaustive-depseslint ruleWhy?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
isSelectedCustomto theuseEffectdependency arrayisSelectedCustominuseCallbackto allow for a stable reference between renders, rather than having the function get redeclared each time.Testing Instructions
exhaustive-depseslint rule #41166, OR manually set'react-hooks/color-palette': 'warn'in your local eslint filenpx eslint packages/components/src/color-palette