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

NavigatorScreen: satisfy exhaustive-deps eslint rule #45648

Conversation

flootr
Copy link
Contributor

@flootr flootr commented Nov 9, 2022

What?

Ensure NavigatorScreen satisfy the exhaustive-deps eslint rule

Why?

Part of the effort in #41166 to apply the exhaustive-deps eslint rule to the components package

How?

In the useEffect call we're manipulating the location object (stored in context) by setting hasRestoredFocus to a boolean value. Due to that assignment the exlint rule expects the whole location object to be a dependency of the useEffect call which isn't what we want. Therefore we use a workaround by storing the location object in ref locationRef and access/write to it that way.

Testing Instructions

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

Screenshots or screencast

@codesandbox
Copy link

codesandbox bot commented Nov 9, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@ciampo ciampo self-assigned this Nov 9, 2022
@ciampo ciampo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Nov 9, 2022
@ciampo ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Nov 9, 2022
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.

Changes LGTM 🚀

I tested in particular for the regression fixed in #44972, everything worked as expected.

Could you please add a CHANGELOG entry ?

@ciampo ciampo enabled auto-merge (squash) November 9, 2022 21:53
@ciampo ciampo merged commit 7fd1dc5 into WordPress:trunk Nov 9, 2022
WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Nov 9, 2022
@github-actions github-actions bot added this to the Gutenberg 14.6 milestone Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
2 participants