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

Iterate on the design of the colors and gradients panel #35535

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

youknowriad
Copy link
Contributor

Related to #34574

This PR updates the color panel a bit:

  • Uses a "ToggleControlGroup" instead of "ButtonGroup" for the switcher between "solid" and "gradient"
  • Uses "VStack" to space things properly

Screen Shot 2021-10-12 at 9 20 23 AM

This impacts both the global styles panel (background color) and blocks (cover block for instance)

@youknowriad youknowriad added the [Type] Enhancement A suggestion for improvement. label Oct 12, 2021
@youknowriad youknowriad self-assigned this Oct 12, 2021
@youknowriad youknowriad requested a review from ellatrix as a code owner October 12, 2021 08:21
@github-actions
Copy link

github-actions bot commented Oct 12, 2021

Size Change: +30 B (0%)

Total Size: 1.07 MB

Filename Size Change
build/block-editor/index.min.js 134 kB +31 B (0%)
build/block-editor/style-rtl.css 13.9 kB +2 B (0%)
build/block-editor/style.css 13.9 kB +1 B (0%)
build/edit-site/style-rtl.css 5.5 kB -2 B (0%)
build/edit-site/style.css 5.5 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 931 B
build/admin-manifest/index.min.js 1.09 kB
build/annotations/index.min.js 2.7 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.08 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.2 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
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 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 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 474 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 600 B
build/block-library/blocks/button/style.css 600 B
build/block-library/blocks/buttons/editor-rtl.css 315 B
build/block-library/blocks/buttons/editor.css 315 B
build/block-library/blocks/buttons/style-rtl.css 370 B
build/block-library/blocks/buttons/style.css 370 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 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 206 B
build/block-library/blocks/columns/editor.css 205 B
build/block-library/blocks/columns/style-rtl.css 497 B
build/block-library/blocks/columns/style.css 496 B
build/block-library/blocks/cover/editor-rtl.css 477 B
build/block-library/blocks/cover/editor.css 480 B
build/block-library/blocks/cover/style-rtl.css 1.17 kB
build/block-library/blocks/cover/style.css 1.17 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 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 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 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 977 B
build/block-library/blocks/gallery/editor.css 982 B
build/block-library/blocks/gallery/style-rtl.css 1.6 kB
build/block-library/blocks/gallery/style.css 1.59 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 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 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 731 B
build/block-library/blocks/image/editor.css 730 B
build/block-library/blocks/image/style-rtl.css 502 B
build/block-library/blocks/image/style.css 505 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 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 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 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 493 B
build/block-library/blocks/media-text/style.css 490 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 568 B
build/block-library/blocks/navigation-link/editor.css 570 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 300 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/style-rtl.css 195 B
build/block-library/blocks/navigation-submenu/style.css 195 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.72 kB
build/block-library/blocks/navigation/editor.css 1.72 kB
build/block-library/blocks/navigation/style-rtl.css 1.62 kB
build/block-library/blocks/navigation/style.css 1.61 kB
build/block-library/blocks/navigation/view.min.js 2.74 kB
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 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 198 B
build/block-library/blocks/page-list/style.css 198 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/editor-rtl.css 210 B
build/block-library/blocks/post-author/editor.css 210 B
build/block-library/blocks/post-author/style-rtl.css 182 B
build/block-library/blocks/post-author/style.css 181 B
build/block-library/blocks/post-comments-form/style-rtl.css 140 B
build/block-library/blocks/post-comments-form/style.css 140 B
build/block-library/blocks/post-comments/style-rtl.css 360 B
build/block-library/blocks/post-comments/style.css 359 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 396 B
build/block-library/blocks/post-featured-image/editor.css 397 B
build/block-library/blocks/post-featured-image/style-rtl.css 156 B
build/block-library/blocks/post-featured-image/style.css 156 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 391 B
build/block-library/blocks/post-template/style.css 392 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 60 B
build/block-library/blocks/post-title/style.css 60 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 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 378 B
build/block-library/blocks/pullquote/style.css 378 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 262 B
build/block-library/blocks/query-pagination/editor.css 255 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query-title/editor-rtl.css 85 B
build/block-library/blocks/query-title/editor.css 85 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 220 B
build/block-library/blocks/quote/theme.css 222 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 374 B
build/block-library/blocks/search/style.css 375 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 250 B
build/block-library/blocks/separator/style.css 250 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 769 B
build/block-library/blocks/site-logo/editor.css 769 B
build/block-library/blocks/site-logo/style-rtl.css 165 B
build/block-library/blocks/site-logo/style.css 165 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 165 B
build/block-library/blocks/social-link/editor.css 165 B
build/block-library/blocks/social-links/editor-rtl.css 812 B
build/block-library/blocks/social-links/editor.css 811 B
build/block-library/blocks/social-links/style-rtl.css 1.3 kB
build/block-library/blocks/social-links/style.css 1.3 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 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 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 636 B
build/block-library/blocks/template-part/editor.css 635 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/term-description/editor-rtl.css 90 B
build/block-library/blocks/term-description/editor.css 90 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 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 853 B
build/block-library/common.css 849 B
build/block-library/editor-rtl.css 9.8 kB
build/block-library/editor.css 9.79 kB
build/block-library/index.min.js 148 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/style-rtl.css 10.4 kB
build/block-library/style.css 10.4 kB
build/block-library/theme-rtl.css 665 B
build/block-library/theme.css 669 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 45.7 kB
build/components/index.min.js 217 kB
build/components/style-rtl.css 15.2 kB
build/components/style.css 15.2 kB
build/compose/index.min.js 10.4 kB
build/core-data/index.min.js 12.4 kB
build/customize-widgets/index.min.js 11.2 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 614 B
build/data/index.min.js 7.1 kB
build/date/index.min.js 31.5 kB
build/deprecated/index.min.js 428 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.46 kB
build/edit-navigation/index.min.js 15.3 kB
build/edit-navigation/style-rtl.css 3.74 kB
build/edit-navigation/style.css 3.74 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 29.3 kB
build/edit-post/style-rtl.css 7.2 kB
build/edit-post/style.css 7.19 kB
build/edit-site/index.min.js 29.6 kB
build/edit-widgets/index.min.js 15.7 kB
build/edit-widgets/style-rtl.css 4.1 kB
build/edit-widgets/style.css 4.1 kB
build/editor/index.min.js 37.5 kB
build/editor/style-rtl.css 3.76 kB
build/editor/style.css 3.75 kB
build/element/index.min.js 3.17 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 5.93 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.6 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.72 kB
build/keycodes/index.min.js 1.3 kB
build/list-reusable-blocks/index.min.js 1.85 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 845 B
build/nux/index.min.js 2.03 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.83 kB
build/primitives/index.min.js 921 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/redux-routine/index.min.js 2.63 kB
build/reusable-blocks/index.min.js 2.19 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.52 kB
build/shortcode/index.min.js 1.48 kB
build/token-list/index.min.js 562 B
build/url/index.min.js 1.74 kB
build/viewport/index.min.js 1.02 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.11 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

What a world of a difference this makes. Before:
Screenshot 2021-10-12 at 10 34 03

After:

Screenshot 2021-10-12 at 10 34 40

Code looks good to me, and if you have high confidence in it, ship it. Otherwise, I'd encourage a quick additional sanity check.

@youknowriad
Copy link
Contributor Author

I'm getting a weird unit test failure on the unit test (Reakit warning), maybe related to ToggleGroupControl component. Any idea @ntsekouras @ciampo

@mtias mtias added the [Feature] UI Components Impacts or related to the UI component system label Oct 12, 2021
@ciampo ciampo self-requested a review October 12, 2021 10:40
@ciampo
Copy link
Contributor

ciampo commented Oct 12, 2021

I'm getting a weird unit test failure on the unit test (Reakit warning), maybe related to ToggleGroupControl component. Any idea @ntsekouras @ciampo

I can see this error on the CI unit tests task:

FAIL packages/block-editor/src/components/colors-gradients/test/control.js
  ● ColorPaletteControl › renders tabs if it is possible to select a color and a gradient rendering a color picker at the start

    expect(jest.fn()).not.toHaveWarned(expected)

    Expected mock function not to be called but it was called with:
    ["Can't determine if the element is a native tabbable element because `ref` wasn't passed to the component.", "
    ", "See reakit.io/docs/tabbable"],["Can't determine if the element is a native tabbable element because `ref` wasn't passed to the component.", "
    ", "See reakit.io/docs/tabbable"],["Can't determine if the element is a native tabbable element because `ref` wasn't passed to the component.", "
    ", "See reakit.io/docs/tabbable"],["Can't determine whether the element is a native radio because `ref` wasn't passed to the component", "
    ", "See reakit.io/docs/radio"],["Can't focus composite item component because `ref` wasn't passed to component.", "
    ", "See reakit.io/docs/composite"],["Can't determine whether the element is a native radio because `ref` wasn't passed to the component", "
    ", "See reakit.io/docs/radio"],["Can't focus composite item component because `ref` wasn't passed to component.", "
    ", "See reakit.io/docs/composite"]

      34 | 	function assertExpectedCalls() {
      35 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 36 | 			expect( console ).not[ matcherName ]();
         | 			^
      37 | 		}
      38 | 	}
      39 | 

      at Object.assertExpectedCalls (packages/jest-console/src/index.js:36:4)

Although I'm not able to reproduce it locally, which makes it hard to debug.

 PASS  packages/block-editor/src/components/colors-gradients/test/control.js (13.911 s)
  ColorPaletteControl
    ✓ renders tabs if it is possible to select a color and a gradient rendering a color picker at the start (30 ms)
    ✓ renders the color picker and does not render tabs if it is only possible to select a color (19 ms)
    ✓ renders the gradient picker and does not render tabs if it is only possible to select a gradient (75 ms)

Test Suites: 1 passed, 1 total
Tests:       3 passed, 3 total
Snapshots:   0 total
Time:        14.703 s

Maybe @diegohaz has any insights? Could it be related to some dependency version mismatch?

@diegohaz
Copy link
Member

I'm getting a weird unit test failure on the unit test (Reakit warning), maybe related to ToggleGroupControl component. Any idea @ntsekouras @ciampo

I can see this error on the CI unit tests task:

FAIL packages/block-editor/src/components/colors-gradients/test/control.js
  ● ColorPaletteControl › renders tabs if it is possible to select a color and a gradient rendering a color picker at the start

    expect(jest.fn()).not.toHaveWarned(expected)

    Expected mock function not to be called but it was called with:
    ["Can't determine if the element is a native tabbable element because `ref` wasn't passed to the component.", "
    ", "See reakit.io/docs/tabbable"],["Can't determine if the element is a native tabbable element because `ref` wasn't passed to the component.", "
    ", "See reakit.io/docs/tabbable"],["Can't determine if the element is a native tabbable element because `ref` wasn't passed to the component.", "
    ", "See reakit.io/docs/tabbable"],["Can't determine whether the element is a native radio because `ref` wasn't passed to the component", "
    ", "See reakit.io/docs/radio"],["Can't focus composite item component because `ref` wasn't passed to component.", "
    ", "See reakit.io/docs/composite"],["Can't determine whether the element is a native radio because `ref` wasn't passed to the component", "
    ", "See reakit.io/docs/radio"],["Can't focus composite item component because `ref` wasn't passed to component.", "
    ", "See reakit.io/docs/composite"]

      34 | 	function assertExpectedCalls() {
      35 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 36 | 			expect( console ).not[ matcherName ]();
         | 			^
      37 | 		}
      38 | 	}
      39 | 

      at Object.assertExpectedCalls (packages/jest-console/src/index.js:36:4)

Although I'm not able to reproduce it locally, which makes it hard to debug.

 PASS  packages/block-editor/src/components/colors-gradients/test/control.js (13.911 s)
  ColorPaletteControl
    ✓ renders tabs if it is possible to select a color and a gradient rendering a color picker at the start (30 ms)
    ✓ renders the color picker and does not render tabs if it is only possible to select a color (19 ms)
    ✓ renders the gradient picker and does not render tabs if it is only possible to select a gradient (75 ms)

Test Suites: 1 passed, 1 total
Tests:       3 passed, 3 total
Snapshots:   0 total
Time:        14.703 s

Maybe @diegohaz has any insights? Could it be related to some dependency version mismatch?

Hmm, that's weird. I couldn't find anything that could be causing this warning in the code. So my best guess is that it's something with react-test-renderer. I know react-test-renderer had some issues with React refs in the past, but they should be fixed by now.

@ntsekouras
Copy link
Contributor

I don't Reakit code but could this something related here: https://github.com/reakit/reakit/blob/master/packages/reakit/src/Radio/Radio.ts#L100?

In Reakit ref is set initially to null and then we have an empty array deps in useEffect... -cc @diegohaz

@diegohaz
Copy link
Member

I converted the first test to use @testing-library/react and the warnings disappeared (dbe40cf). Let's see how the CI responds.

@diegohaz
Copy link
Member

diegohaz commented Oct 12, 2021

I don't Reakit code but could this something related here: https://github.com/reakit/reakit/blob/master/packages/reakit/src/Radio/Radio.ts#L100?

In Reakit ref is set initially to null and then we have an empty array deps in useEffect... -cc @diegohaz

That's correct. The ref will be assigned after the component is mounted, but before effects run. So the effect callback should have access to the ref value. Since the ref is stable, there's no need to add it to the effect deps either.

I guess there may be a problem between react-test-renderer and the async act. The effect is probably running before the ref is assigned in the tests.

@youknowriad
Copy link
Contributor Author

Thanks all for your help here :)

@youknowriad youknowriad merged commit 7aa8979 into trunk Oct 12, 2021
@youknowriad youknowriad deleted the update/colors-gradients-panel branch October 12, 2021 15:36
@github-actions github-actions bot added this to the Gutenberg 11.8 milestone Oct 12, 2021
isSmall
isPressed={ currentTab === 'gradient' }
onClick={ () => setCurrentTab( 'gradient' ) }
<VStack space={ 3 }>
Copy link
Contributor

@ciampo ciampo Oct 14, 2021

Choose a reason for hiding this comment

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

While working with this component in a completely different PR, I realised that the correct prop name was spacing (see docs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be solved in #35583

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

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 [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants