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

UnitControl: fix exhaustive-deps warnings #44161

Merged
merged 4 commits into from
Sep 19, 2022

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Sep 14, 2022

What?

Part of #41166

Fix all exhaustive-deps warnings on the UnitControl component.

Why?

See #41166 for more context

How?

Add missing deps on the web component, add eslint-disable comments on the native component

Testing Instructions

  • Run npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/unit-control/ and make sure that no warnings are printed
  • Test the UnitControl component in both Storybook and the editor, in particular play around with setting/unsetting units. Make sure that the component behaves as expected.

@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Sep 14, 2022
@ciampo ciampo self-assigned this Sep 14, 2022
@github-actions
Copy link

github-actions bot commented Sep 14, 2022

Size Change: +4 B (0%)

Total Size: 1.26 MB

Filename Size Change
build/components/index.min.js 198 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 982 B
build/annotations/index.min.js 2.76 kB
build/api-fetch/index.min.js 2.26 kB
build/autop/index.min.js 2.14 kB
build/blob/index.min.js 475 B
build/block-directory/index.min.js 7.05 kB
build/block-directory/style-rtl.css 990 B
build/block-directory/style.css 991 B
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/index.min.js 163 kB
build/block-editor/style-rtl.css 15.4 kB
build/block-editor/style.css 15.4 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 126 B
build/block-library/blocks/audio/theme.css 126 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 84 B
build/block-library/blocks/avatar/style.css 84 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 482 B
build/block-library/blocks/button/editor.css 482 B
build/block-library/blocks/button/style-rtl.css 523 B
build/block-library/blocks/button/style.css 523 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 100 B
build/block-library/blocks/categories/style.css 100 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 103 B
build/block-library/blocks/code/style.css 103 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 187 B
build/block-library/blocks/comment-template/style.css 185 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 834 B
build/block-library/blocks/comments/editor.css 832 B
build/block-library/blocks/comments/style-rtl.css 632 B
build/block-library/blocks/comments/style.css 630 B
build/block-library/blocks/cover/editor-rtl.css 605 B
build/block-library/blocks/cover/editor.css 607 B
build/block-library/blocks/cover/style-rtl.css 1.57 kB
build/block-library/blocks/cover/style.css 1.55 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 253 B
build/block-library/blocks/file/style.css 254 B
build/block-library/blocks/file/view.min.js 346 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 948 B
build/block-library/blocks/gallery/editor.css 950 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 B
build/block-library/blocks/group/editor-rtl.css 337 B
build/block-library/blocks/group/editor.css 337 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 327 B
build/block-library/blocks/html/editor.css 329 B
build/block-library/blocks/image/editor-rtl.css 876 B
build/block-library/blocks/image/editor.css 873 B
build/block-library/blocks/image/style-rtl.css 627 B
build/block-library/blocks/image/style.css 630 B
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 463 B
build/block-library/blocks/latest-posts/style.css 462 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 507 B
build/block-library/blocks/media-text/style.css 505 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 705 B
build/block-library/blocks/navigation-link/editor.css 703 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation-submenu/view.min.js 423 B
build/block-library/blocks/navigation/editor-rtl.css 1.99 kB
build/block-library/blocks/navigation/editor.css 2 kB
build/block-library/blocks/navigation/style-rtl.css 2.17 kB
build/block-library/blocks/navigation/style.css 2.16 kB
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 443 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 363 B
build/block-library/blocks/page-list/editor.css 363 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 174 B
build/block-library/blocks/paragraph/editor.css 174 B
build/block-library/blocks/paragraph/style-rtl.css 260 B
build/block-library/blocks/paragraph/style.css 260 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 493 B
build/block-library/blocks/post-comments-form/style.css 493 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 547 B
build/block-library/blocks/post-featured-image/editor.css 545 B
build/block-library/blocks/post-featured-image/style-rtl.css 315 B
build/block-library/blocks/post-featured-image/style.css 315 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 282 B
build/block-library/blocks/post-template/style.css 282 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 326 B
build/block-library/blocks/pullquote/style.css 325 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 282 B
build/block-library/blocks/query-pagination/style.css 278 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 439 B
build/block-library/blocks/query/editor.css 439 B
build/block-library/blocks/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 409 B
build/block-library/blocks/search/style.css 406 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 464 B
build/block-library/blocks/shortcode/editor.css 464 B
build/block-library/blocks/site-logo/editor-rtl.css 488 B
build/block-library/blocks/site-logo/editor.css 488 B
build/block-library/blocks/site-logo/style-rtl.css 203 B
build/block-library/blocks/site-logo/style.css 203 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.4 kB
build/block-library/blocks/social-links/style.css 1.39 kB
build/block-library/blocks/spacer/editor-rtl.css 322 B
build/block-library/blocks/spacer/editor.css 322 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 494 B
build/block-library/blocks/table/editor.css 494 B
build/block-library/blocks/table/style-rtl.css 611 B
build/block-library/blocks/table/style.css 609 B
build/block-library/blocks/table/theme-rtl.css 190 B
build/block-library/blocks/table/theme.css 190 B
build/block-library/blocks/tag-cloud/style-rtl.css 239 B
build/block-library/blocks/tag-cloud/style.css 239 B
build/block-library/blocks/template-part/editor-rtl.css 235 B
build/block-library/blocks/template-part/editor.css 235 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 561 B
build/block-library/blocks/video/editor.css 563 B
build/block-library/blocks/video/style-rtl.css 174 B
build/block-library/blocks/video/style.css 174 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/common-rtl.css 1.02 kB
build/block-library/common.css 1.02 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 11 kB
build/block-library/editor.css 11 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 190 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 12.2 kB
build/block-library/style.css 12.2 kB
build/block-library/theme-rtl.css 719 B
build/block-library/theme.css 722 B
build/block-serialization-default-parser/index.min.js 1.1 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 49.6 kB
build/components/style-rtl.css 11.4 kB
build/components/style.css 11.5 kB
build/compose/index.min.js 12.5 kB
build/core-data/index.min.js 15.5 kB
build/customize-widgets/index.min.js 11.3 kB
build/customize-widgets/style-rtl.css 1.38 kB
build/customize-widgets/style.css 1.38 kB
build/data-controls/index.min.js 653 B
build/data/index.min.js 8.08 kB
build/date/index.min.js 32.1 kB
build/deprecated/index.min.js 507 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.7 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 3.99 kB
build/edit-navigation/style.css 4 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/index.min.js 30.8 kB
build/edit-post/style-rtl.css 6.94 kB
build/edit-post/style.css 6.94 kB
build/edit-site/index.min.js 58 kB
build/edit-site/style-rtl.css 8.36 kB
build/edit-site/style.css 8.34 kB
build/edit-widgets/index.min.js 16.5 kB
build/edit-widgets/style-rtl.css 4.34 kB
build/edit-widgets/style.css 4.34 kB
build/editor/index.min.js 41.7 kB
build/editor/style-rtl.css 3.66 kB
build/editor/style.css 3.65 kB
build/element/index.min.js 4.68 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 6.76 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.64 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.77 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.78 kB
build/keycodes/index.min.js 1.81 kB
build/list-reusable-blocks/index.min.js 1.74 kB
build/list-reusable-blocks/style-rtl.css 835 B
build/list-reusable-blocks/style.css 835 B
build/media-utils/index.min.js 2.93 kB
build/notices/index.min.js 953 B
build/nux/index.min.js 2.05 kB
build/nux/style-rtl.css 732 B
build/nux/style.css 728 B
build/plugins/index.min.js 1.94 kB
build/preferences-persistence/index.min.js 2.22 kB
build/preferences/index.min.js 1.3 kB
build/primitives/index.min.js 933 B
build/priority-queue/index.min.js 1.58 kB
build/react-i18n/index.min.js 696 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.74 kB
build/reusable-blocks/index.min.js 2.21 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.6 kB
build/server-side-render/index.min.js 1.61 kB
build/shortcode/index.min.js 1.53 kB
build/style-engine/index.min.js 1.45 kB
build/token-list/index.min.js 644 B
build/url/index.min.js 3.61 kB
build/vendors/react-dom.min.js 38.5 kB
build/vendors/react.min.js 4.34 kB
build/viewport/index.min.js 1.09 kB
build/warning/index.min.js 268 B
build/widgets/index.min.js 7.18 kB
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.19 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

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.

Looks good to me 🚀

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Just noting that this now makes the effect also fire each time the number is changed, where previously it was only when the unit is changed. Would that cause any problems?

Might be worth trying to stabilize the state setter in useControlledState itself?

@@ -100,7 +100,7 @@ function UnforwardedUnitControl(
if ( parsedUnit !== undefined ) {
setUnit( parsedUnit );
}
}, [ parsedUnit ] );
}, [ parsedUnit, setUnit ] );
Copy link
Member

Choose a reason for hiding this comment

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

Ooh, TIL about this useControlledState thing. Looks useful. I just saw the same pattern in ToggleGroupControl, but without using this utility hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

And there's also useControlledValue 🤦 It would be good to see if we can use only one implementation across the whole package

@ciampo
Copy link
Contributor Author

ciampo commented Sep 15, 2022

Just noting that this now makes the effect also fire each time the number is changed, where previously it was only when the unit is changed. Would that cause any problems?

Might be worth trying to stabilize the state setter in useControlledState itself?

I can look into it and see what happens!

@ciampo ciampo force-pushed the fix/unit-control-exhaustive-deps branch from d88f29d to a4324af Compare September 19, 2022 15:35
@ciampo
Copy link
Contributor Author

ciampo commented Sep 19, 2022

Might be worth trying to stabilize the state setter in useControlledState itself?

Done in a4324af

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Works as expected 👍

@ciampo ciampo merged commit 5d00e5a into trunk Sep 19, 2022
@ciampo ciampo deleted the fix/unit-control-exhaustive-deps branch September 19, 2022 17:02
@github-actions github-actions bot added this to the Gutenberg 14.2 milestone Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants