Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components: refactor BorderControl to pass exhaustive-deps #41259

Merged
merged 3 commits into from
May 24, 2022

Conversation

chad1008
Copy link
Contributor

What?

Updates the BorderControl component to satisfy the exhaustive-deps eslint rule

Why?

Part of the effort in #41166 to apply exhuastive-deps to the Components package

How?

  • remove sanitizeBorder from dependency array. sanitizeBorder isn't a prop, and mutating it doesn't rerender the component so it's not a valid dependency
  • add colorSelection and styleSelection to dependency array

Testing Instructions

  1. Rebase this PR on top of Components: Add exhaustive-deps eslint rule #41166, OR manually set
    'react-hooks/exhaustive-deps': 'warn' in your local eslint file
  2. From your local Gutenberg directory, run npx eslint packages/components/src/border-control
  3. Confirm that the linter returns no errors
  4. Launch Storybook
  5. Test the component, any stories, and its docs to ensure everything still works as expected.

@chad1008 chad1008 requested review from mirka and ciampo May 23, 2022 19:25
@chad1008 chad1008 marked this pull request as ready for review May 23, 2022 19:25
@chad1008 chad1008 requested a review from ajitbohra as a code owner May 23, 2022 19:25
@chad1008 chad1008 changed the title Components: refactor BorderControl to pass exhaustive-deps Components: refactor BorderControl to pass exhaustive-deps May 23, 2022
@@ -56,7 +56,7 @@ export function useBorderControl(

onChange( newBorder );
},
[ onChange, shouldSanitizeBorder, sanitizeBorder ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronrobertshaw , do you remember if there's any particular reason why you added sanitizeBorder to the list of dependencies here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Honestly can't recall and looks like I added it before tidying up local experimentation and drafting the initial PR.

I probably just had it in my head that changing sanitizeBorder would mean a changed callback. It makes sense that coming from an outer scope it isn't a valid dependency and should be removed.

@ciampo ciampo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components [Feature] Component System WordPress component system labels May 23, 2022
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @chad1008 👍

✅ No linting errors
BorderControl and BorderBoxControl continue to function in Storybook
✅ Border controls within the editor work correctly

LGTM! 🚢

@@ -56,7 +56,7 @@ export function useBorderControl(

onChange( newBorder );
},
[ onChange, shouldSanitizeBorder, sanitizeBorder ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Honestly can't recall and looks like I added it before tidying up local experimentation and drafting the initial PR.

I probably just had it in my head that changing sanitizeBorder would mean a changed callback. It makes sense that coming from an outer scope it isn't a valid dependency and should be removed.

@stokesman
Copy link
Contributor

Just chiming in to say it looks like the testing instructions for these PRs could be simplified with command line overrides. I tested: npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/border-control and it did the trick. No need to rebase or add the rule to config.

@ciampo ciampo merged commit eca485f into trunk May 24, 2022
@ciampo ciampo deleted the enhance/Bordercontrol-exhuastive-deps branch May 24, 2022 11:32
@github-actions github-actions bot added this to the Gutenberg 13.4 milestone May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants