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 RangeControl to pass exhaustive-deps #44271

Merged
merged 8 commits into from
Sep 21, 2022

Conversation

chad1008
Copy link
Contributor

@chad1008 chad1008 commented Sep 19, 2022

What?

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

Why?

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

How?

tooltip.tsx:

  • inputRef should always be a ref object passed in by the consumer, even React can't tell that it is. As a ref, it'll stay stable and a safe dependency.

utils.ts:

  • useControlledRangeValue setInternalState is declared by running the useControlledState hook. I didn't notice any adverse effects to this new dependency, and the underlying function is also being memoized in UnitControl: fix exhaustive-deps warnings #44161
  • useDebouncedHoverInteraction
    • it seems that this hook was intended to show the tooltip on mouse enter (move) and hide the tooltip on mouse leave, but currently the RangeControl component is showing/hiding the Tooltip component on focus/blur.
    • Therefore, it appears that the useDebouncedHoverInteraction hook was simply out of date and can be deleted without causing any consequences
    • The onShowTooltip and onHideTooltip props from InputRangeProps were also deleted since they were only used when passed to the useDebouncedHoverInteraction prop, and are therefore not used anymore

Testing Instructions

  1. From your local Gutenberg directory, run npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/range-control
  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

@chad1008 chad1008 self-assigned this Sep 19, 2022
@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 Sep 19, 2022
@ciampo ciampo changed the title Components: refactor BorderControl to pass exhaustive-deps Components: refactor RangeControl to pass exhaustive-deps Sep 20, 2022
@ciampo ciampo marked this pull request as ready for review September 20, 2022 16:12
@ciampo
Copy link
Contributor

ciampo commented Sep 20, 2022

Hey @mirka @aaronrobertshaw and @tyxla , @chad1008 and I paired on this PR and therefore would appreciate if you could take a look and review it! Thanks 🙏

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 the ping to review this one 👍

I'll need to remove the useDebouncedHoverInteraction custom hook and reflect these same changes for the new SliderControl component in #42315.

As for this PR, it tests well for me in the storybook and editor.

✅ No build or type errors
✅ No general linting errors
✅ No exhaustive-deps warnings for the RangeControl
✅ RangeControl unit tests pass
✅ Storybook: RangeControl and docs function correctly
✅ RangeControls in the editor work as expected

@ciampo ciampo merged commit 148c816 into trunk Sep 21, 2022
@ciampo ciampo deleted the refactor/range-control-exhaustive-deps branch September 21, 2022 07:20
@github-actions github-actions bot added this to the Gutenberg 14.2 milestone Sep 21, 2022
@chad1008 chad1008 changed the title Components: refactor RangeControl to pass exhaustive-deps Components: refactor ResizeBox to pass exhaustive-deps Sep 22, 2022
@chad1008 chad1008 changed the title Components: refactor ResizeBox to pass exhaustive-deps Components: refactor RangeControl to pass exhaustive-deps Sep 22, 2022
aaronrobertshaw added a commit that referenced this pull request Sep 23, 2022
This reflects the changes made in #44271 for the RangeControl.
aaronrobertshaw added a commit that referenced this pull request Oct 3, 2022
This reflects the changes made in #44271 for the RangeControl.
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.

3 participants