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.

Sign up for GitHub

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

Jump to bottom

Components: refactor NavigatorButton to pass exhaustive-deps #42051

Merged
merged 2 commits into from
Sep 5, 2022
Merged

Components: refactor NavigatorButton to pass exhaustive-deps #42051

merged 2 commits into from
Sep 5, 2022

Conversation

flootr
Copy link
Contributor

@flootr flootr commented Jun 29, 2022 •
edited
Loading

What?

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

Why?

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

How?

Add the attributeName prop & escapedName to the useCallback dependency array.

Testing Instructions

  1. From your local Gutenberg directory, run npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/navigation/index.js
  2. Confirm that the linter returns no errors
  3. Confirm unit tests still pass
  4. Run Storybook locally, confirm the components stories and/or docs still work as expected

@stokesman stokesman mentioned this pull request Jul 16, 2022
Merged
44 tasks
@flootr flootr force-pushed the enhance/NavigatorButton-exhaustive-deps branch from bd67a8c to 96a5926 Compare August 29, 2022 13:41
@flootr flootr marked this pull request as ready for review August 29, 2022 13:42
@flootr flootr requested a review from ajitbohra as a code owner August 29, 2022 13:42
@chad1008 chad1008 added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components [Feature] Component System WordPress component system labels Aug 29, 2022
chad1008
chad1008 approved these changes Aug 29, 2022
View reviewed changes
Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

Code changes look good to me, Storybook and unit tests all going well! @flootr can you please add a CHANGELOG entry before merging? 🙂

@flootr flootr force-pushed the enhance/NavigatorButton-exhaustive-deps branch from 96a5926 to 1437019 Compare August 30, 2022 08:35
@flootr
Copy link
Contributor Author

flootr commented Aug 30, 2022 •
edited
Loading

@chad1008 added via 1437019 👍🏻

ciampo
ciampo approved these changes Aug 31, 2022
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Let's merge this once conflicts have been solved and CI checks are ✅

@flootr flootr force-pushed the enhance/NavigatorButton-exhaustive-deps branch from 1437019 to 8a48f2d Compare September 1, 2022 11:06
tyxla
tyxla approved these changes Sep 5, 2022
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Tested well for me as well 👍

@ciampo ciampo merged commit f821819 into WordPress:trunk Sep 5, 2022
@github-actions github-actions bot added this to the Gutenberg 14.1 milestone Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chad1008 chad1008 chad1008 approved these changes

@tyxla tyxla tyxla approved these changes

@ciampo ciampo ciampo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@flootr flootr

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
Milestone
Gutenberg 14.1
Development

Successfully merging this pull request may close these issues.
4 participants
@flootr @ciampo @tyxla @chad1008