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

Query: Fix add new post link position via private SlotFill #49819

Merged
merged 10 commits into from
Apr 19, 2023

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Apr 14, 2023

Fixes: #49327

What?

  • Updates the SlotFill component to allow Symbol keys instead of string names only.
  • Creates a small util function to generate a SlotFill using a Symbol key
  • Creates a "private" SlotFill for rendering additional block information (BlockInfo).
  • Exports the BlockInfo component as a private API in the block-editor package
  • Renders the BlockInfo slot in the block inspector
  • Updates the Query block to import and unlock the the new BlockInfo fill.
  • Renames the "Create new post" link to "Add new post" and renders it as a BlockInfo fill.

🤖 Generated by Copilot at e46f7af

This pull request adds a new BlockInfo component that can show information about the selected block in the block toolbar. It uses a private slot-fill pair to render the component in different parts of the block editor UI. It also updates the query block UI to use the new component and improves the slot-fill module to support symbols as keys.

Why?

When plugins register Block Styles for the Query block the Block Styles UI is rendered above the "Add new post" link which should sit immediately below the Block Card or the block's variation transforms if it has them.

We can't use a normal SlotFill as anyone with that name could abuse the slot to render anything above the tabs undermining the proper prominence for controls.

The proposed "private" SlotFill is only private as far as our Private/Experimental API can make it. A determined individual can still unlock experimental or private APIs even though they shouldn't. It has been generally accepted that we can still consider anything locked via the private API, to in fact be private for our purposes.

The ability to use a private SlotFill allows us to avoid introducing block-specific code into the block-editor package simply to restore the position of the Query block's link.

How?

  • Created a new private SlotFill for block information
  • Exported private SlotFill as a private API
  • Updated the Block Inspector to render the new slot
  • Updated the Query block to unlock the new SlotFill and render its "Add new post" link as a fill for it

It might be easiest to review the PR commit-by-commit as they are all pretty small and self-contained.

Next Steps

  • Write some tests for SlotFills using Symbols as keys

Testing Instructions

  1. Add some block styles to the Query block (example in the snippet below)
  2. Edit a post, add a query block, select it and confirm the incorrect create new post link position
  3. Checkout this PR branch, rebuild, and reload the post
  4. The create new post link position should now be below the block card but above the block styles
<?php
/**
 * Register block styles.
 */
function blue_note_register_block_styles() {
	register_block_style(
		'core/query',
		array(
			'name'  => 'blue-note-query-slant',
			'label' => __( 'Slanted images', 'blue-note' ),
			'inline_style' => '.is-style-blue-note-query-slant .wp-block-post-featured-image {transform: rotate(-1deg);}',
		)
	);
}
add_action( 'init', 'blue_note_register_block_styles' );

Testing Instructions for Keyboard

Screenshots or screencast

Before After
Screenshot 2023-04-04 at 3 53 24 pm Screenshot 2023-04-14 at 3 42 10 pm

@aaronrobertshaw aaronrobertshaw added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Block] Query Loop Affects the Query Loop Block labels Apr 14, 2023
@aaronrobertshaw aaronrobertshaw self-assigned this Apr 14, 2023
@github-actions
Copy link

github-actions bot commented Apr 14, 2023

Size Change: +45 B (0%)

Total Size: 1.37 MB

Filename Size Change
build/block-editor/index.min.js 203 kB +87 B (0%)
build/block-library/index.min.js 204 kB -98 B (0%)
build/components/index.min.js 208 kB +56 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/annotations/index.min.js 2.78 kB
build/api-fetch/index.min.js 2.27 kB
build/autop/index.min.js 2.15 kB
build/blob/index.min.js 483 B
build/block-directory/index.min.js 7.2 kB
build/block-directory/style-rtl.css 1.04 kB
build/block-directory/style.css 1.04 kB
build/block-editor/content-rtl.css 4.17 kB
build/block-editor/content.css 4.17 kB
build/block-editor/default-editor-styles-rtl.css 403 B
build/block-editor/default-editor-styles.css 403 B
build/block-editor/style-rtl.css 14.7 kB
build/block-editor/style.css 14.7 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 90 B
build/block-library/blocks/archives/style.css 90 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 138 B
build/block-library/blocks/audio/theme.css 138 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 91 B
build/block-library/blocks/avatar/style.css 91 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 587 B
build/block-library/blocks/button/editor.css 587 B
build/block-library/blocks/button/style-rtl.css 628 B
build/block-library/blocks/button/style.css 627 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 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 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 121 B
build/block-library/blocks/code/style.css 121 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 409 B
build/block-library/blocks/columns/style.css 409 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 199 B
build/block-library/blocks/comment-template/style.css 198 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 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 649 B
build/block-library/blocks/cover/editor.css 651 B
build/block-library/blocks/cover/style-rtl.css 1.61 kB
build/block-library/blocks/cover/style.css 1.6 kB
build/block-library/blocks/details-summary/editor-rtl.css 65 B
build/block-library/blocks/details-summary/editor.css 65 B
build/block-library/blocks/details-summary/style-rtl.css 61 B
build/block-library/blocks/details-summary/style.css 61 B
build/block-library/blocks/details/style-rtl.css 54 B
build/block-library/blocks/details/style.css 54 B
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 138 B
build/block-library/blocks/embed/theme.css 138 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 269 B
build/block-library/blocks/file/style.css 270 B
build/block-library/blocks/file/view.min.js 353 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 984 B
build/block-library/blocks/gallery/editor.css 988 B
build/block-library/blocks/gallery/style-rtl.css 1.55 kB
build/block-library/blocks/gallery/style.css 1.55 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 654 B
build/block-library/blocks/group/editor.css 654 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 340 B
build/block-library/blocks/html/editor.css 341 B
build/block-library/blocks/image/editor-rtl.css 830 B
build/block-library/blocks/image/editor.css 829 B
build/block-library/blocks/image/style-rtl.css 652 B
build/block-library/blocks/image/style.css 652 B
build/block-library/blocks/image/theme-rtl.css 137 B
build/block-library/blocks/image/theme.css 137 B
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 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 478 B
build/block-library/blocks/latest-posts/style.css 478 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 716 B
build/block-library/blocks/navigation-link/editor.css 715 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 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation/editor-rtl.css 2.13 kB
build/block-library/blocks/navigation/editor.css 2.14 kB
build/block-library/blocks/navigation/style-rtl.css 2.22 kB
build/block-library/blocks/navigation/style.css 2.21 kB
build/block-library/blocks/navigation/view-modal.min.js 2.81 kB
build/block-library/blocks/navigation/view.min.js 447 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 401 B
build/block-library/blocks/page-list/editor.css 401 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 279 B
build/block-library/blocks/paragraph/style.css 281 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 501 B
build/block-library/blocks/post-comments-form/style.css 501 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 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 588 B
build/block-library/blocks/post-featured-image/editor.css 586 B
build/block-library/blocks/post-featured-image/style-rtl.css 322 B
build/block-library/blocks/post-featured-image/style.css 322 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 281 B
build/block-library/blocks/post-template/style.css 281 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 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 335 B
build/block-library/blocks/pullquote/style.css 335 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 288 B
build/block-library/blocks/query-pagination/style.css 284 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 463 B
build/block-library/blocks/query/editor.css 463 B
build/block-library/blocks/quote/style-rtl.css 222 B
build/block-library/blocks/quote/style.css 222 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 149 B
build/block-library/blocks/rss/editor.css 149 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 408 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 329 B
build/block-library/blocks/shortcode/editor.css 329 B
build/block-library/blocks/site-logo/editor-rtl.css 489 B
build/block-library/blocks/site-logo/editor.css 489 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 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 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 359 B
build/block-library/blocks/spacer/editor.css 359 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 433 B
build/block-library/blocks/table/editor.css 433 B
build/block-library/blocks/table/style-rtl.css 651 B
build/block-library/blocks/table/style.css 650 B
build/block-library/blocks/table/theme-rtl.css 157 B
build/block-library/blocks/table/theme.css 157 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 404 B
build/block-library/blocks/template-part/editor.css 404 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 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 179 B
build/block-library/blocks/video/style.css 179 B
build/block-library/blocks/video/theme-rtl.css 139 B
build/block-library/blocks/video/theme.css 139 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.12 kB
build/block-library/common.css 1.12 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.6 kB
build/block-library/editor.css 11.6 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 12.8 kB
build/block-library/style.css 12.8 kB
build/block-library/theme-rtl.css 698 B
build/block-library/theme.css 703 B
build/block-serialization-default-parser/index.min.js 1.13 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 51.1 kB
build/commands/index.min.js 14.8 kB
build/commands/style-rtl.css 789 B
build/commands/style.css 786 B
build/components/style-rtl.css 11.7 kB
build/components/style.css 11.7 kB
build/compose/index.min.js 12.4 kB
build/core-data/index.min.js 16.3 kB
build/customize-widgets/index.min.js 12.2 kB
build/customize-widgets/style-rtl.css 1.41 kB
build/customize-widgets/style.css 1.41 kB
build/data-controls/index.min.js 718 B
build/data/index.min.js 8.68 kB
build/date/index.min.js 40.4 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.76 kB
build/edit-post/classic-rtl.css 571 B
build/edit-post/classic.css 571 B
build/edit-post/index.min.js 35 kB
build/edit-post/style-rtl.css 7.61 kB
build/edit-post/style.css 7.6 kB
build/edit-site/index.min.js 64.5 kB
build/edit-site/style-rtl.css 10.1 kB
build/edit-site/style.css 10.1 kB
build/edit-widgets/index.min.js 17.3 kB
build/edit-widgets/style-rtl.css 4.56 kB
build/edit-widgets/style.css 4.56 kB
build/editor/index.min.js 45.9 kB
build/editor/style-rtl.css 3.49 kB
build/editor/style.css 3.48 kB
build/element/index.min.js 4.95 kB
build/escape-html/index.min.js 548 B
build/format-library/index.min.js 7.26 kB
build/format-library/style-rtl.css 557 B
build/format-library/style.css 556 B
build/hooks/index.min.js 1.66 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.79 kB
build/is-shallow-equal/index.min.js 535 B
build/keyboard-shortcuts/index.min.js 1.79 kB
build/keycodes/index.min.js 1.94 kB
build/list-reusable-blocks/index.min.js 2.14 kB
build/list-reusable-blocks/style-rtl.css 865 B
build/list-reusable-blocks/style.css 865 B
build/media-utils/index.min.js 2.99 kB
build/notices/index.min.js 977 B
build/plugins/index.min.js 1.94 kB
build/preferences-persistence/index.min.js 2.23 kB
build/preferences/index.min.js 1.35 kB
build/primitives/index.min.js 960 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 942 B
build/react-i18n/index.min.js 702 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.75 kB
build/reusable-blocks/index.min.js 2.26 kB
build/reusable-blocks/style-rtl.css 265 B
build/reusable-blocks/style.css 265 B
build/rich-text/index.min.js 11.1 kB
build/server-side-render/index.min.js 2.09 kB
build/shortcode/index.min.js 1.52 kB
build/style-engine/index.min.js 1.55 kB
build/token-list/index.min.js 650 B
build/url/index.min.js 3.74 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 1.09 kB
build/warning/index.min.js 280 B
build/widgets/index.min.js 7.3 kB
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

It's nice that this didn't require too many modifications. I think being able to create private slots will be really useful.

I checked out the branch and tried a few things to see if there were any ways to render in the slot without unlocking the private API:

  • <Fill name="BlockInformation">TEST</Fill> - didn't work as expected.
  • <Fill name={ Symbol.for( 'BlockInformation' ) }>Test</BlockInformation> - didn't work as expected, since the symbols created for the slot fill aren't in the symbol registry.

It'd be good to get thoughts from @youknowriad and @adamziel on this approach.

packages/components/src/slot-fill/index.js Outdated Show resolved Hide resolved
packages/block-library/src/query/index.js Outdated Show resolved Hide resolved
@aaronrobertshaw
Copy link
Contributor Author

Thanks for the early review @talldan 🚀

A lot of the feedback is on my radar for further polishing before making this ready for final review. The plan is to address it all first thing next week.

That said, getting some early feedback, sanity checking just how "private" the SlotFill would be sounds wise. Thanks for sending out the pings 🙇

@youknowriad
Copy link
Contributor

Since this is all private, I don't mind it too much but it seems to me that if BlockStyles were also rendered using the BlockInspector slot/fills (which we already have there). One could still use BlockInspector (and hook priorities) to inject UI where this PR is proposing BlockInfo no?

@aaronrobertshaw
Copy link
Contributor Author

Thanks for weighing in @youknowriad 👍

seems to me that if BlockStyles were also rendered using the BlockInspector slot/fills (which we already have there). One could still use BlockInspector (and hook priorities) to inject UI where this PR is proposing BlockInfo no?

Unfortunately, no, unless I'm missing something.

If we rely on rendering the Block Styles into existing InspectorControls slots, we can't guarantee order and end up with 3rd parties easily being able to push their controls to a higher prominence than warranted and above the Block Styles.

If we were to add a new InspectorControls group slot for the block styles and explicitly position that slot above the default InspectorControls slot, we end up back where we started needing the means to allow the Query block to position its additional block info (i.e. the new post link) above the Block Styles.

The current approach in this PR only adds a single new SlotFill. I don't think having an additional private slot dedicated to the Block Styles only provides much value.

@youknowriad
Copy link
Contributor

If I'm not mistake @mtias had some resistance in the past to adding such a slot or URLs to blocks there. So, I'm just looping him in just in case.

@aaronrobertshaw
Copy link
Contributor Author

If I'm not mistake @mtias had some resistance in the past to adding such a slot or URLs to blocks there. So, I'm just looping him in just in case.

I'm happy to wait for @mtias to weigh in, if needed.

For historical context, Matías was involved in #45437 where we explored a new slot when first rolling out the Block Inspector tabs. The concerns there were the slot being abused by 3rd parties. It was this concern that the private SlotFill aims to mitigate.

It's my understanding we can accept that the private APIs lock/unlock feature is sufficient to consider the new SlotFill private and secure enough.

@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review April 18, 2023 01:49
@aaronrobertshaw aaronrobertshaw changed the title [WIP] Query: Fix add new post link position via private SlotFill Query: Fix add new post link position via private SlotFill Apr 18, 2023
@@ -115,6 +116,7 @@ function BlockInspectorLockedBlocks( { topLevelLockedBlock } ) {
className={ blockInformation.isSynced && 'is-synced' }
/>
<BlockVariationTransforms blockClientId={ topLevelLockedBlock } />
<BlockInfo.Slot />
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be above BlockVariationTransforms? Also related issue about slot in BlockCard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the block inspector tabs were being introduced, we explored adding a new slot above the tabs for items that related to the block as a whole but weren't really a setting or style. At that time the direction was that it should occur the block transforms as those should still have a higher prominence.

In the Query block's use case with the "add new post" link, it doesn't particularly matter.

It's worth noting that the earlier exploration into adding a slot above the block inspector tabs was blocked due to concerns about the slot being abused by 3rd parties that might promote their controls in ways detrimental to the end-user experience. The potential "resistance" to a new slot fill here was questioned again in earlier comments on this PR. It was these concerns that drove the approach in this PR to make this proposed slot "private".

A "private" slot as proposed here wouldn't solve the need expressed in #49887 as the use case there requires it to be available for public consumption.

So that leaves me a little unsure of how best to proceed here. Some options are:

  1. Proceed with this private slot approach to solve the Query block's immediate need.
    • Buys time to get consensus on if we are in fact happy to add a public slot
    • Allows time for exploration to see if we might mitigate misuse of the public slot by filtering allowed fills or something
    • It can easily be changed or removed should we land a new public slot and the private slot fills might be beneficial elsewhere.
  2. Shelve this fix until we get consensus on a new public slot, including what is acceptable to be added above the block transforms
  3. Bite the bullet and add a public slot to the BlockCard now
  4. Rework this PR to only add the private SlotFill feature omitting its use in the block inspector and query block for now

While I don't feel strongly, I'm leaning more towards option 1, landing this PR and iterating from there. That leaning was reinforced further after some internal discussions with @talldan.

What do you think? Are there any better options I haven't considered?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thank you Aaron!

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

I think it's worth landing this. There's been so much back and forth on this tiny piece of UI, it's been a lot of effort for little reward. I don't think it's worth stretching it out even further.

The implementation here looks good, it avoids introducing code that has tech debt and it can all be removed in the future if a better solution comes along, I personally see no issues.

@aaronrobertshaw aaronrobertshaw enabled auto-merge (squash) April 19, 2023 07:51
@aaronrobertshaw aaronrobertshaw merged commit 404ba01 into trunk Apr 19, 2023
@aaronrobertshaw aaronrobertshaw deleted the try/private-slot-fills branch April 19, 2023 08:14
@github-actions github-actions bot added this to the Gutenberg 15.7 milestone Apr 19, 2023
@adamziel
Copy link
Contributor

adamziel commented Apr 19, 2023

Late to the party but I like this implementation and I'm glad to see that lock() is flexible enough to support all sorts of use-cases. Great use of symbols, too.

@ciampo
Copy link
Contributor

ciampo commented May 9, 2023

Out of curiosity, is creating a "private slotfill" API necessary? Could we just rely on having the locked BlockInfo slot and using symbols directly in the BlockInfo implementation?

@aaronrobertshaw
Copy link
Contributor Author

is creating a "private slotfill" API necessary?

I’d say it would be possible to avoid the private SlotFill API. The BlockInfo implementation would then need to import the Fill and Slot directly and reimplement the createSlotFill util to handle symbols. If we keep the updates to createSlotFill in the components package, then createPrivateSlotFill is a minor addition.

Private SlotFills have already been added in a few more places within Gutenberg, I think providing the API will help reduce code duplication and increase consistency & convenience.

If the API isn’t something that you think fits alongside the component package’s SlotFill API, we could look at relocating it.

@ciampo
Copy link
Contributor

ciampo commented May 15, 2023

Private SlotFills have already been added in a few more places within Gutenberg

Thank you for the extra context, I was not aware of this.

If the API isn’t something that you think fits alongside the component package’s SlotFill API, we could look at relocating it.

My comment above was mostly trying to gather more context and understand if we were talking about a single exception, or if the "private slot fill" is a repeated pattern that is worth addressing at the components' package level. I'm happy to keep it as is if we feel that's a correct decision.

@youknowriad , what's your stance, as someone with tons of experience on slot fill ?

@gziolo
Copy link
Member

gziolo commented May 29, 2023

We can't use a normal SlotFill as anyone with that name could abuse the slot to render anything above the tabs undermining the proper prominence for controls.

I agree with the sentiment, but at the same time, it was previously possible to hack the description field and include additional DOM nodes, as explained in #49887. It is also doable to use InspectorControls in combination with editor.blockEdit filter to render anything the plugin author wants very close to the top of the sidebar – it was implemented this way before.

I would be in favor of opening this new slot for every plugin author. If necessary, it can be wrapped into an HTML element like <p> with opinionated styling that makes it harder to put any type of content inside.

@gziolo gziolo mentioned this pull request May 29, 2023
67 tasks
@mtias
Copy link
Member

mtias commented May 31, 2023

One thing to consider is that block info / description can be used in multiple places where you wouldn't expect to be extended — for example, on block previews that include the block description the link for the query block would never be accessible. It seems the semantics should be closely tied to the inspector rendering of this unit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Loop Affects the Query Loop Block [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
8 participants