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

InspectorControls: Wrap block support slots in ToolsPanel #34157

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Aug 19, 2021

Depends on:

Related:

Description

  • Extends the work done in Block Editor: Use groups for InspectorControls #34069 to provide groups in InspectorControls.
  • Adds a slot for the dimensions block support
  • Adds the new slot into the block inspector
  • Wraps block support InspectorControl groups in a ToolsPanel.
    • Does this via new component that handles the generation of resetAll function, getting block attributes from the store etc.
  • Updates dimensions block support hook to inject its padding and margin controls via the hook

How has this been tested?

  1. Enable padding and margin support in theme.json
  2. Opt into margin support for the group block ( we need two to properly test )
  3. Edit a post, add a group block and select it
  4. Confirm that the dimensions panel appears in the sidebar
  5. Toggle the padding and margin controls on and off and ensure they display correctly
  6. Set values for both padding and margin. Then test resetting them individually via the panel's menu
  7. Set both values again before this time selecting "reset all" from the panel's menu
  8. Ensure both values have been reset and the block has updated correctly.

Screenshots

Types of changes

New feature. Enhancement.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@aaronrobertshaw aaronrobertshaw added [Feature] Block API API that allows to express the block paradigm. [Package] Block editor /packages/block-editor [Type] New API New API to be used by plugin developers or package users. [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Aug 19, 2021
@aaronrobertshaw aaronrobertshaw self-assigned this Aug 19, 2021
@gziolo gziolo force-pushed the update/inspector-controls-groups branch from 435415b to e3309e8 Compare August 19, 2021 08:26
@github-actions
Copy link

github-actions bot commented Aug 19, 2021

Size Change: +156 B (0%)

Total Size: 1.04 MB

Filename Size Change
build/block-editor/index.min.js 120 kB +167 B (0%)
build/components/index.min.js 209 kB -2 B (0%)
build/edit-site/index.min.js 26.6 kB -9 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.19 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-editor/style-rtl.css 13.8 kB
build/block-editor/style.css 13.8 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 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 666 B
build/block-library/blocks/cover/editor.css 670 B
build/block-library/blocks/cover/style-rtl.css 1.23 kB
build/block-library/blocks/cover/style.css 1.23 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 927 B
build/block-library/blocks/gallery/editor.css 934 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 70 B
build/block-library/blocks/group/theme.css 70 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 283 B
build/block-library/blocks/html/editor.css 284 B
build/block-library/blocks/image/editor-rtl.css 728 B
build/block-library/blocks/image/editor.css 728 B
build/block-library/blocks/image/style-rtl.css 482 B
build/block-library/blocks/image/style.css 487 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 488 B
build/block-library/blocks/media-text/style.css 485 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 489 B
build/block-library/blocks/navigation-link/editor.css 488 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/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.42 kB
build/block-library/blocks/navigation/style.css 1.41 kB
build/block-library/blocks/navigation/view.min.js 2.52 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 310 B
build/block-library/blocks/page-list/editor.css 310 B
build/block-library/blocks/page-list/style-rtl.css 241 B
build/block-library/blocks/page-list/style.css 241 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 261 B
build/block-library/blocks/paragraph/style.css 261 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-content/editor-rtl.css 138 B
build/block-library/blocks/post-content/editor.css 138 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 398 B
build/block-library/blocks/post-featured-image/editor.css 398 B
build/block-library/blocks/post-featured-image/style-rtl.css 143 B
build/block-library/blocks/post-featured-image/style.css 143 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 378 B
build/block-library/blocks/post-template/style.css 379 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 270 B
build/block-library/blocks/query-pagination/editor.css 262 B
build/block-library/blocks/query-pagination/style-rtl.css 239 B
build/block-library/blocks/query-pagination/style.css 236 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 462 B
build/block-library/blocks/site-logo/editor.css 464 B
build/block-library/blocks/site-logo/style-rtl.css 153 B
build/block-library/blocks/site-logo/style.css 153 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.54 kB
build/block-library/editor.css 9.52 kB
build/block-library/index.min.js 151 kB
build/block-library/reset-rtl.css 527 B
build/block-library/reset.css 527 B
build/block-library/style-rtl.css 10.2 kB
build/block-library/style.css 10.2 kB
build/block-library/theme-rtl.css 658 B
build/block-library/theme.css 663 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 46.9 kB
build/components/style-rtl.css 15.8 kB
build/components/style.css 15.8 kB
build/compose/index.min.js 10.2 kB
build/core-data/index.min.js 12.3 kB
build/customize-widgets/index.min.js 11.1 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.53 kB
build/edit-navigation/index.min.js 14.9 kB
build/edit-navigation/style-rtl.css 3.37 kB
build/edit-navigation/style.css 3.37 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 29 kB
build/edit-post/style-rtl.css 7.2 kB
build/edit-post/style.css 7.2 kB
build/edit-site/style-rtl.css 5.07 kB
build/edit-site/style.css 5.07 kB
build/edit-widgets/index.min.js 16.1 kB
build/edit-widgets/style-rtl.css 4.06 kB
build/edit-widgets/style.css 4.06 kB
build/editor/index.min.js 37.7 kB
build/editor/style-rtl.css 3.74 kB
build/editor/style.css 3.73 kB
build/element/index.min.js 3.17 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 5.36 kB
build/format-library/style-rtl.css 668 B
build/format-library/style.css 669 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.49 kB
build/keycodes/index.min.js 1.25 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.88 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.28 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.32 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.27 kB
build/widgets/style-rtl.css 1.17 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I like this approach personally. But you worked on both, you'd have a better insight here, what are your current thoughts?

@gziolo
Copy link
Member

gziolo commented Aug 19, 2021

Just letting you know that applied some breaking changes to #34069 and therefore to the base branch used here. The only important change is that group was marked as experimental based on the feedback received, so it's now __experimentalGroup.

@aaronrobertshaw aaronrobertshaw force-pushed the try/inspector-controls-with-toolspanel branch from b1b16a2 to 7c23fd8 Compare August 20, 2021 02:49
@aaronrobertshaw
Copy link
Contributor Author

I like this approach personally. But you worked on both, you'd have a better insight here, what are your current thoughts?

This approach will make the block support hooks a little cleaner, reducing some boilerplate code. For the immediate uses planned, I like this approach as well.

If a scenario does arise in the future where a block support panel needs to do something more than just reset attributes, we can cross that bridge then.

While creating this PR I did run into some odd behaviour resetting the block attributes via updateBlockAttributes instead of the setAttributes passed through props. That ultimately led to tightening up the ToolsPanel which is another win.

I'll close the original PR in favour of this one.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is testing well for me @aaronrobertshaw, I like this approach, and nice work fixing up the resetAll behaviour!

I was wondering if it's worth us adding a constants.js file to limit using string literals in the __experimentalGroup prop for dimensions, border, color, typography etc? It's definitely something we could look into in a follow-up instead (so not blocking for this PR), but just to help avoid accidental typos in subsequent PRs, which are sometimes tricky to debug. This comment might be more applicable to #34069 of course, so I'll add a comment over there, too.

Otherwise, looks great to me!

@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review August 20, 2021 06:46
@gziolo gziolo force-pushed the update/inspector-controls-groups branch from 83bc87e to 8312f54 Compare August 20, 2021 08:03
Base automatically changed from update/inspector-controls-groups to trunk August 23, 2021 07:39
@aaronrobertshaw aaronrobertshaw force-pushed the try/inspector-controls-with-toolspanel branch from 515f33d to 852267f Compare August 24, 2021 07:50
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Good work @aaronrobertshaw, this is testing well for me, and nice idea using the panelId to ensure that injected ToolsPanelItems are connected to the correct ToolsPanel. It works well in testing, and as we discussed in #34065 (comment), once the __experimentalGroup prop stabilises, it'd be good for us to update the InspectorControls documentation with examples on how to use the ToolsPanel item and hook it up to the right ToolsPanel via the clientId. Since this is still experimental, I think it's fine to delay getting the docs in as we're likely to change implementation details over time.

The behaviour in this PR and #34065 feels solid to me in testing, and I think it includes some good pragmatic workarounds for the edge cases we encountered, so I'm giving it the ✅, but let's see what others think about the panelId idea, too.

@aaronrobertshaw
Copy link
Contributor Author

@gziolo or @youknowriad do you have any other thoughts or concerns I need to address on this PR? It would be great to sneak it, and the related cover block PR, into this release if possible.

In particular, I thought you might have some better ideas around how I solved the issues where switching between block selections still had previous fills hanging around which caused incorrect panel items to be registered.

@aaronrobertshaw aaronrobertshaw added the [Feature] UI Components Impacts or related to the UI component system label Sep 7, 2021
@aaronrobertshaw
Copy link
Contributor Author

Sorry for the late review, but I've only just realised this PR was in the works!

It's very much appreciated. My apologies for missing the components label on this PR and specific components squad members from the reviewers.

Also, it'd be great if in the future we could avoid mixing block editor and @wordpress/components changes within the same PR

Understood. The changes here evolved from issues uncovered when attempting to implement the use of the ToolsPanel around the new grouped InspectorControls slots. These were also in a separate branch when this PR was first created, so splintering this again at that stage was problematic.

With the majority of those issues debugged and addressed. I'd be happy to split out the ToolsPanel updates from this PR if it will help move this all forward. I'll also be more aware of isolating component changes moving forward. Thanks.

@aaronrobertshaw aaronrobertshaw force-pushed the try/inspector-controls-with-toolspanel branch from f058696 to 574c27b Compare September 8, 2021 06:04
@aaronrobertshaw aaronrobertshaw changed the base branch from trunk to update/tools-panel-for-slotfills-use September 8, 2021 06:05
@aaronrobertshaw aaronrobertshaw force-pushed the try/inspector-controls-with-toolspanel branch from 574c27b to e9bbbed Compare September 8, 2021 06:06
@aaronrobertshaw aaronrobertshaw removed the [Feature] UI Components Impacts or related to the UI component system label Sep 8, 2021
@aaronrobertshaw
Copy link
Contributor Author

The updates to the ToolsPanel required to facilitate support for injecting panel items via SlotFills have been split into a separate PR #34632.

This PR now targets #34632 as its base.

@gziolo
Copy link
Member

gziolo commented Sep 8, 2021

In general, it'd be good to keep the components squad in the loop every time a @wordpress/components component gets edited — and especially when the APIs are modified.

I'm afraid it isn't a realistic expectation in the project that has over 800 contributors. There isn't even a formal WordPress team for @wordpress/components that the community could learn about. Fortuntately there are ways to get better overview about all the changes proposed in PRs:

  • everyone willing to review changed to packages/components folder can add their GitHub handles to the CODOWNERS file to get notification when PR gets opened that modified files in the folder:
    /packages/components @ajitbohra @jaymanpandya @chrisvanpatten
  • we discussed it in the past, but we could build GitHub action that automatically applies labels based on the type of modified files
  • more time-consuming approach is to go through the list of commits and opened PRs and scan for related changed on a regular schedule (week or month)
  • even when something get merged, it doesn't mean it can't be reverted or updated

Also, it'd be great if in the future we could avoid mixing block editor and @wordpress/components changes within the same PR

Can you expand on this point? I don't think I agree on principle with this limitation, but many I don't know the full story.

@gziolo
Copy link
Member

gziolo commented Sep 8, 2021

It looks like we need the following steps to move forward with this PR:

  1. ToolsPanel: Allow SlotFill injection of panel items #34632 waits for review from @ciampo, once it's approved we can merge and rebase this PR with trunk
  2. We seem to agree that this PR can land as is after point 1 happens.
  3. I discussed with @aaronrobertshaw that he will open a follow-up PR after point 2 happens. The scope will be limited to the changes to the SlotFill implementation as discussed in InspectorControls: Wrap block support slots in ToolsPanel #34157 (comment). I'm happy to help with finding a working solution. @diegohaz and everyone else who find it intriguing you are all welcome to join efforts 😄

@ciampo
Copy link
Contributor

ciampo commented Sep 8, 2021

Can you expand on this point? I don't think I agree on principle with this limitation, but many I don't know the full story.

There are a few reasons behind my previous suggestion about keeping changes to the @wordpress/components package separate from the rest of the changes.

Firstly, splitting PRs into smaller chunks makes them easier to review and, in particular, isolating changes to the @wordpress/components package makes it easier to search/track/understand them at a later time (for example, for tracking down the root cause of a bug and issuing a fix).

But most importantly, the @wordpress/components package aims at providing a base for building UIs in Gutenberg (and beyond). Changes made to this package need to be considered more carefully compared to standard "product" work, since they will have repercussions for all of the users of this package (and not only the Gutenberg repo). This is even more important when considering how much we value not introducing breaking changes.

The @wordpress/components package still contains legacy components that are not always consistent with each other and that feel like "copy-pasted app components", lacking that level of polish and thoughtfulness that should be part of a good components library. Our recent efforts, like updating the contributing guidelines and investigating APIs consistency, aimed at improving the situation in these areas.

With this perspective in mind, I don't see this being a limitation — rather an improvement in how we collaborate on the @wordpress/components package, which should help to increase its overall quality and consistency.

This should hopefully also give more context on the other point about involving the components squad:

I'm afraid it isn't a realistic expectation in the project that has over 800 contributors. There isn't even a formal WordPress team for @wordpress/components that the community could learn about.

I agree that this is not realistic, and I quite like some of the ideas that you suggested. Ultimately we need to think about what's the best and most scalable solution.

@gziolo
Copy link
Member

gziolo commented Sep 8, 2021

Firstly, splitting PRs into smaller chunks makes them easier to review and, in particular, isolating changes to the @wordpress/components package makes it easier to search/track/understand them at a later time (for example, for tracking down the root cause of a bug and issuing a fix).

It depends on the perspective. From the development perspective it's easier to operate on a single branch as you can better drive the API decision which you can see in the discussions that happened in this PR. Splitting the work into smaller PRs to land changes seems like a good middle ground here if that helps to review the changes to @wordpress/components as discussed above.

The @wordpress/components package still contains legacy components that are not always consistent with each other and that feel like "copy-pasted app components", lacking that level of polish and thoughtfulness that should be part of a good components library.

100% agree. This is exactly how this collection of components was born and evolved. That's why G2 components project has been started to address all the pain points you outlined and I personally love all the efforts towards making APIs unified and better organized 😄

Base automatically changed from update/tools-panel-for-slotfills-use to trunk September 10, 2021 01:08
Co-Authored-By: Greg Ziółkowski <grzegorz@gziolo.pl>
@aaronrobertshaw aaronrobertshaw force-pushed the try/inspector-controls-with-toolspanel branch from e9bbbed to 269b5d1 Compare September 10, 2021 01:18
@andrewserong
Copy link
Contributor

Just gave this is another smoke test, and it's working nicely for me with the useEffect added back in for clearing the isResetting value. Looks like that now ensures that clearing the value happens after the render is completed, instead of part way through 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Package] Block editor /packages/block-editor [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants