Skip to content

NavigatorScreen: satisfy exhaustive-deps eslint rule#45648

Merged
ciampo merged 2 commits intoWordPress:trunkfrom
flootr:refactor/navigator-screen/has-restored-focus-exhaustive-dep
Nov 9, 2022
Merged

NavigatorScreen: satisfy exhaustive-deps eslint rule#45648
ciampo merged 2 commits intoWordPress:trunkfrom
flootr:refactor/navigator-screen/has-restored-focus-exhaustive-dep

Conversation

@flootr
Copy link
Copy Markdown
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

@flootr flootr requested a review from ajitbohra as a code owner November 9, 2022 18:37
@codesandbox
Copy link
Copy Markdown

codesandbox bot commented Nov 9, 2022

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

@ciampo ciampo requested review from chad1008, ciampo, mirka and tyxla November 9, 2022 21:27
@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
Copy link
Copy Markdown
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
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants