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

Use the Button block inside of the Search block #39463

Closed
wants to merge 4 commits into from

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Mar 15, 2022

Description

This PR replaces the <button /> element inside of the Search block for a Button block. It is a step towards consolidating the CSS required to render the Search block inside of the search block (overview issue). There are two major downsides of using an HTML element instead of a block:

The solution proposed here comes with its own set of challenges.

Most notably, it is unclear how to reconcile granular control over the button block with the search block attributes such as isButtonPositionInside or buttonUseIcon. It was simple with a controlled HTML <button />, but with a Button block the user may set buttonUseIcon to true on the Search block and then click on said icon inside the Button block and remove it. Should it be disallowed? Should that icon be restored after setting buttonUseIcon to false and then back to true?

cc @mtias @scruffian @getdave @draganescu @gziolo @dmsnell

@adamziel adamziel marked this pull request as draft March 15, 2022 13:59
@github-actions
Copy link

github-actions bot commented Mar 15, 2022

Size Change: +1.12 kB (0%)

Total Size: 1.16 MB

Filename Size Change
build/block-editor/index.min.js 145 kB +209 B (0%)
build/block-editor/style-rtl.css 15 kB +5 B (0%)
build/block-editor/style.css 15 kB +6 B (0%)
build/block-library/blocks/gallery/editor-rtl.css 961 B -4 B (0%)
build/block-library/blocks/gallery/editor.css 964 B -3 B (0%)
build/block-library/blocks/gallery/style-rtl.css 1.51 kB -101 B (-6%)
build/block-library/blocks/gallery/style.css 1.51 kB -103 B (-6%)
build/block-library/editor-rtl.css 9.96 kB -4 B (0%)
build/block-library/editor.css 9.96 kB -5 B (0%)
build/block-library/index.min.js 169 kB +695 B (0%)
build/block-library/style-rtl.css 11.2 kB -122 B (-1%)
build/block-library/style.css 11.3 kB -122 B (-1%)
build/blocks/index.min.js 46.8 kB +289 B (+1%)
build/components/index.min.js 218 kB +320 B (0%)
build/components/style-rtl.css 15.6 kB +15 B (0%)
build/components/style.css 15.6 kB +12 B (0%)
build/core-data/index.min.js 14.3 kB +122 B (+1%)
build/customize-widgets/index.min.js 11.2 kB -18 B (0%)
build/data/index.min.js 8.19 kB +141 B (+2%)
build/edit-navigation/index.min.js 16.1 kB -27 B (0%)
build/edit-post/index.min.js 29.8 kB -1 B (0%)
build/edit-site/index.min.js 43.8 kB -138 B (0%)
build/edit-widgets/index.min.js 16.5 kB -44 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/admin-manifest/index.min.js 1.24 kB
build/annotations/index.min.js 2.77 kB
build/api-fetch/index.min.js 2.27 kB
build/autop/index.min.js 2.15 kB
build/blob/index.min.js 487 B
build/block-directory/index.min.js 6.49 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 150 B
build/block-library/blocks/audio/editor.css 150 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 445 B
build/block-library/blocks/button/editor.css 445 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 292 B
build/block-library/blocks/buttons/editor.css 292 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 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 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-template/style-rtl.css 127 B
build/block-library/blocks/comment-template/style.css 127 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-query-loop/editor-rtl.css 95 B
build/block-library/blocks/comments-query-loop/editor.css 95 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.56 kB
build/block-library/blocks/cover/style.css 1.56 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 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 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/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/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 529 B
build/block-library/blocks/image/style.css 535 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 199 B
build/block-library/blocks/latest-posts/editor.css 198 B
build/block-library/blocks/latest-posts/style-rtl.css 447 B
build/block-library/blocks/latest-posts/style.css 446 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 708 B
build/block-library/blocks/navigation-link/editor.css 706 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 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/view.min.js 375 B
build/block-library/blocks/navigation/editor-rtl.css 2.03 kB
build/block-library/blocks/navigation/editor.css 2.04 kB
build/block-library/blocks/navigation/style-rtl.css 1.89 kB
build/block-library/blocks/navigation/style.css 1.88 kB
build/block-library/blocks/navigation/view.min.js 2.85 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 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 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/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/style-rtl.css 446 B
build/block-library/blocks/post-comments-form/style.css 446 B
build/block-library/blocks/post-comments/style-rtl.css 521 B
build/block-library/blocks/post-comments/style.css 521 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 721 B
build/block-library/blocks/post-featured-image/editor.css 721 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/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 323 B
build/block-library/blocks/post-template/style.css 323 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 80 B
build/block-library/blocks/post-title/style.css 80 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 389 B
build/block-library/blocks/pullquote/style.css 388 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 234 B
build/block-library/blocks/query-pagination/style.css 231 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 201 B
build/block-library/blocks/quote/style.css 201 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 397 B
build/block-library/blocks/search/style.css 398 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 233 B
build/block-library/blocks/separator/style.css 233 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 744 B
build/block-library/blocks/site-logo/editor.css 744 B
build/block-library/blocks/site-logo/style-rtl.css 181 B
build/block-library/blocks/site-logo/style.css 181 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 177 B
build/block-library/blocks/social-link/editor.css 177 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.37 kB
build/block-library/blocks/social-links/style.css 1.36 kB
build/block-library/blocks/spacer/editor-rtl.css 332 B
build/block-library/blocks/spacer/editor.css 332 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 226 B
build/block-library/blocks/tag-cloud/style.css 227 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 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 934 B
build/block-library/common.css 932 B
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/theme-rtl.css 665 B
build/block-library/theme.css 670 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/compose/index.min.js 11.2 kB
build/customize-widgets/style-rtl.css 1.39 kB
build/customize-widgets/style.css 1.39 kB
build/data-controls/index.min.js 663 B
build/date/index.min.js 31.9 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.53 kB
build/edit-navigation/style-rtl.css 4.04 kB
build/edit-navigation/style.css 4.05 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/style-rtl.css 7.07 kB
build/edit-post/style.css 7.07 kB
build/edit-site/style-rtl.css 7.44 kB
build/edit-site/style.css 7.42 kB
build/edit-widgets/style-rtl.css 4.39 kB
build/edit-widgets/style.css 4.39 kB
build/editor/index.min.js 38.4 kB
build/editor/style-rtl.css 3.71 kB
build/editor/style.css 3.71 kB
build/element/index.min.js 4.29 kB
build/escape-html/index.min.js 548 B
build/format-library/index.min.js 6.62 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 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.83 kB
build/keycodes/index.min.js 1.41 kB
build/list-reusable-blocks/index.min.js 1.75 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.94 kB
build/notices/index.min.js 957 B
build/nux/index.min.js 2.12 kB
build/nux/style-rtl.css 751 B
build/nux/style.css 749 B
build/plugins/index.min.js 1.98 kB
build/preferences/index.min.js 1.2 kB
build/primitives/index.min.js 949 B
build/priority-queue/index.min.js 611 B
build/react-i18n/index.min.js 704 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.69 kB
build/reusable-blocks/index.min.js 2.24 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11.1 kB
build/server-side-render/index.min.js 1.61 kB
build/shortcode/index.min.js 1.52 kB
build/token-list/index.min.js 668 B
build/url/index.min.js 1.99 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 280 B
build/widgets/index.min.js 7.21 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.07 kB

compressed-size-action

@adamziel adamziel changed the title The initial attempt to bring innerblocks into the search block Use the Button block inside of the Search block Mar 16, 2022
@adamziel
Copy link
Contributor Author

adamziel commented Mar 16, 2022

@mtias said:

The border radius, the padding, etc, should all be connected attributes – you don’t want to modify the vertical padding of the button and not affect the input. You also probably don’t want the button to come before the input, so the ability to change position is also irrelevant.

To which I replied:

All great notes! In this case, these properties shouldn’t really be editable through the button; or if they are, it should get reflected back on the input. I guess we really want for now are the button block styles, not necessarily the entire baggage. The block may change its html structure, though, so we need to run the same code paths. What if the Button block was just an internal implementation detail, not something the user can select or manipulate through the UI?

And updated this PR accordingly. The styles are now synchronized between the search block and the button block. Through trial and error I ended up using controlled inner blocks with templateLock: "all" and they do almost everything we need here. The last piece of this puzzle is to enable editing just the text of that button. I am not sure if this is supported at the moment though, any ideas @Mamaduka @gziolo @scruffian @draganescu @getdave ? Maybe if the button text would be technically a non-locked inner paragraph block?

@scruffian
Copy link
Contributor

I know this doesn't relate to the question you asked, but I noticed that the icon setting is also broken...
Screenshot 2022-03-16 at 15 56 45

@adamziel
Copy link
Contributor Author

@scruffian Ah yes, I didn't get the icon to render so I replaced it with a text for now. That looks like a detail I can fix with more tinkering though, my main problem is with the editable text.

@Mamaduka
Copy link
Member

@adamziel I don't think I've ever done something like this, sorry 😅

Maybe if the button text would be technically a non-locked inner paragraph block?

The button block is a RichText, so we can't use a paragraph block inside it. Even if we could paragraph comes with its settings, we would've to disable them.

To be honest, this looks like a lot of work just to render a block and don't use any of the settings.

@scruffian
Copy link
Contributor

What if we just used the .wp-block-button class on the search button?

@adamziel
Copy link
Contributor Author

Capturing two notes from my conversation with @gziolo: __experimentalCaptureToolbars and __experimentalExposeControlsToChildren. If we could replace the toolbar and the inspector controls of the button with those of the search block, that would work as well.

@gziolo
Copy link
Member

gziolo commented Mar 17, 2022

Capturing two notes from my conversation with @gziolo: __experimentalCaptureToolbars and __experimentalExposeControlsToChildren. If we could replace the toolbar and the inspector controls of the button with those of the search block, that would work as well.

Related merged PR for toolbar controls is #33955.
Related open PR for inspector controls is #34261.

Both render controls explicitly marked in the parent to child blocks. In your case, you need the reverse behavior. Force the child block to render all the block controls for the parent and optionally inject some additional controls from the currently selected child block.

@adamziel
Copy link
Contributor Author

adamziel commented Mar 17, 2022

I ditched the template locks – it seems to be a dead end. I can't think of any API that would us specify control which block features we want locked, and which ones we want to be interactive.

The PRs linked by @gziolo do not fix the problem either, but they are a really good entrypoint to exploring how would the implementation look like. Importantly, we do want to see the Search block toolbar and inspector controls, not the Button block's ones.

I started with an absolutely terrible hack to trick the editor into thinking the search block is selected even when the user clicks on the button block:

export function selection( state = {}, action ) {
	if (
		action.clientId ===
		document.querySelector( '.wp-block-button' )?.dataset?.block
	) {
		action.clientId = document.querySelector(
			'.wp-block-search'
		)?.dataset?.block;
	}

	switch ( action.type ) {
		case 'SELECTION_CHANGE':
			// ...

It only works if you have a single search block in the editor, and it does a surprisingly good job:

CleanShot 2022-03-17 at 17 01 29

For the record, I don't think that updating the reducer this way is a good idea at all. I just wanted to see if the "selection swap" concept has any chances of succeeding – it seems like it does.

What do you think about the idea? If it sounds reasonable, I will try to roll out a proper implementation along the lines of @gziolo's PR linked above. CC @draganescu @getdave @scruffian

@adamziel
Copy link
Contributor Author

adamziel commented Mar 17, 2022

I also considered rendering the button block to an element tree inside of a useEffect call, surgically swapping its content for a RichText field, and finally returning all of that from the Search block's edit function. It seems like a bad idea, though – it would introduce a tight coupling that would easily break.

action.clientId ===
document.querySelector( '.wp-block-button' )?.dataset?.block
) {
action.clientId = document.querySelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

This really looks like a hack 😁 and not sure if there even is a proper implementation of this.

buttonUseIcon,
// Must serialize these two, they are new objects each time and
// keep triggering the effect
JSON.stringify( colorProps ),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird :)

@draganescu
Copy link
Contributor

@adamziel reading through the comments I noticed @scruffian 's suggestion got no answer. Is adding that class not doing much?

@mtias
Copy link
Member

mtias commented Mar 31, 2022

I'd like to add that I believe this is a case where we should definitely not be using child blocks. When thinking about whether inner blocks is the correct mental model to apply to the elements of a block we should think about what child blocks afford: the ability to declare, move, copy, duplicate, reorder, delete on one hand, and the ability to set attributes locally on the other.

Neither of this apply elegantly to this block:

  • You'd only have one "button" at a time — i.e. no "patterns".
  • Ordering is of very dubious value.
  • There's no use in having the inner blocks inserter because the button is either shown or not shown.
  • Most styles are going to be coupled (and you don't want to repeat them as attributes unnecessarily).

When we look at the results we aim to unlock, we also have the icon only button, which has no representation through core/button (and is not currently handled here). The designs that use the icon make the input and button blur with each other, which also speaks towards this being a single block conceptually.

Inheriting styles from the generic core/button becomes a much narrower need, which shouldn't dictate the structure of the block. There are many other examples where we want to capture buttons as elements and not as a block. That's likely closer to what the solution should be — to use Button not as a block but as a component, similar to RichText, expressed as an Element.

@scruffian
Copy link
Contributor

I had a go at adding support for a button element: #40260

@scruffian
Copy link
Contributor

Now #40260 has merged I think we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants