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

[WIP] Add background image support to group block #38784

Closed
wants to merge 9 commits into from

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Feb 14, 2022

Description

As illustrated by the number of 👍 comments on #14744 there is user demand for the ability to set image backgrounds on the Group block.

This has been a long-time outstanding issue and with the standardizing of Global Styles it's now one I think we can look to address.

This PR is currently a WIP which simply copy/pastes the implementation from the Cover block to gauge interest in the feature.

Note: currently this only works in the Editor. The block will not yet output images on the front of the site.

Further work will proceed in phases:

  1. Port over save.js implementation from core/cover to allow for front end to reflect changes in editor.
  2. Refactor shared utils/implementation details to share between Group and Cover (possibly in a separate PR).
  3. Finalise and test.

Feedback required

I'm looking for feedback principally on whether folks still want to proceed with adding background images to the Group block. Is it necessary? What alternatives are there? What technical problems do we need to solve?

One of the main technical problems is that the Group block was adjusted (by @youknowriad) to remove the inner <div> element which we would probably need to restore if we proceed with this PR. Currently I've restored the <div> but we'll need to revisit it.

Testing Instructions

At this stage I'm looking for feedback on feature parity with the Cover block.

  • What doesn't work?
  • What is missing?
  • What features does Group need that Cover doesn't?
  • Any quirks?

Screenshots

Screen.Capture.on.2022-02-14.at.13-53-54.mov

Types of changes

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).
  • I've updated related schemas if appropriate.

@getdave getdave added [Type] Enhancement A suggestion for improvement. [Block] Group Affects the Group Block labels Feb 14, 2022
@github-actions
Copy link

github-actions bot commented Feb 14, 2022

Size Change: +3.04 kB (0%)

Total Size: 1.15 MB

Filename Size Change
build/block-library/blocks/group/style-rtl.css 660 B +603 B (+1058%) 🆘
build/block-library/blocks/group/style.css 660 B +603 B (+1058%) 🆘
build/block-library/index.min.js 169 kB +1.01 kB (+1%)
build/block-library/style-rtl.css 11.7 kB +411 B (+4%)
build/block-library/style.css 11.8 kB +409 B (+4%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/admin-manifest/index.min.js 1.13 kB
build/annotations/index.min.js 2.77 kB
build/api-fetch/index.min.js 2.25 kB
build/autop/index.min.js 2.15 kB
build/blob/index.min.js 487 B
build/block-directory/index.min.js 6.51 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/index.min.js 142 kB
build/block-editor/style-rtl.css 14.8 kB
build/block-editor/style.css 14.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 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 470 B
build/block-library/blocks/button/editor.css 470 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 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 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.55 kB
build/block-library/blocks/cover/style.css 1.55 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 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/editor-rtl.css 965 B
build/block-library/blocks/gallery/editor.css 967 B
build/block-library/blocks/gallery/style-rtl.css 1.61 kB
build/block-library/blocks/gallery/style.css 1.61 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/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 518 B
build/block-library/blocks/image/style.css 523 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 649 B
build/block-library/blocks/navigation-link/editor.css 650 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 1.98 kB
build/block-library/blocks/navigation/editor.css 1.99 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 401 B
build/block-library/blocks/page-list/editor.css 402 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 214 B
build/block-library/blocks/tag-cloud/style.css 215 B
build/block-library/blocks/template-part/editor-rtl.css 560 B
build/block-library/blocks/template-part/editor.css 559 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 921 B
build/block-library/common.css 919 B
build/block-library/editor-rtl.css 10.1 kB
build/block-library/editor.css 10.1 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/theme-rtl.css 672 B
build/block-library/theme.css 676 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 46.4 kB
build/components/index.min.js 215 kB
build/components/style-rtl.css 15.5 kB
build/components/style.css 15.5 kB
build/compose/index.min.js 11.2 kB
build/core-data/index.min.js 14 kB
build/customize-widgets/index.min.js 11.4 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 663 B
build/data/index.min.js 7.47 kB
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/index.min.js 16.2 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/index.min.js 30 kB
build/edit-post/style-rtl.css 7.17 kB
build/edit-post/style.css 7.16 kB
build/edit-site/index.min.js 41.9 kB
build/edit-site/style-rtl.css 7.22 kB
build/edit-site/style.css 7.21 kB
build/edit-widgets/index.min.js 16.7 kB
build/edit-widgets/style-rtl.css 4.17 kB
build/edit-widgets/style.css 4.17 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 3.32 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 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.98 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.25 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.92 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 280 B
build/widgets/index.min.js 7.18 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

@jasmussen
Copy link
Contributor

Impressive and fast work! Took it for a quick spin and seeing this:

spin

There's some overlap with Cover here. Ultimately I do think we'll want background support in the Group block, and possibly many others, but one of the challenges is that the inspector controls for adding and managing backgrounds in the Cover block aren't yet that strong, and could use a great deal of refinement on their own. I recall that both @javierarce and @critterverse have done explorations in this area, and although #38577 deals with patterns, it speculates on other small iterations to the inspector. Group opens up the additional question of what happens when the group is empty? Cover handles that by adding a min-width.

In other words, we know we need to improve the design controls for backgrounds, and it might be worth it to do that before we expand backgrounds to other blocks. That way, when we do we can potentially manage the complexity by making "Background" a hidden-by-default addition to various toolspanels.

Although definitely not as convenient, it seems like Cover could work as an interim solution if you need it. What do you think?

@getdave
Copy link
Contributor Author

getdave commented Feb 15, 2022

Thanks for your review as always @jasmussen 🙇‍♂️

There's some overlap with Cover here.

Oh 100%. There is. But Cover != Group in my opinion. They have different semantics and use cases although I appreciate you are no doubt aware of that.

...one of the challenges is that the inspector controls for adding and managing backgrounds in the Cover block aren't yet that strong,

One of my concerns is that Group now has both background and overlay. Cover only seems to have overlay.

Group opens up the additional question of what happens when the group is empty?

I'd say we leave it to do whatever it does naturally. If you decide to set a background on a Group but then don't add any content to that Group then it should behave as it does now but just with the background image displayed.

In other words, we know we need to improve the design controls for backgrounds, and it might be worth it to do that before we expand backgrounds to other blocks. That way, when we do we can potentially manage the complexity by making "Background" a hidden-by-default addition to various toolspanels.

One of my goals here would have been to refactor some of the tools so that they are resuable and shared between the blocks. I assumed this would probably lead to them being picked up by someone working on Global Styles? Perhaps @jorgefilipecosta can advise on how he sees the situation with background image controls and how these might be abstracted in future.

it seems like Cover could work as an interim solution if you need it. What do you think?

IMHO it feels like we've had this [Cover block] as an "interim" solution for too long now. I raised the Issue in April 2019.

Part of the reason for me raising this PR is to raise awareness of how long the Issue has been outstanding and how many folks are keen to see this feature land. If it turns out there is little appetite however, then we can close this out until background controls mature. I just fear that without something concrete to drive that effort forward we might be back here in another 2 years time 😄

@paaljoachim
Copy link
Contributor

Right now it seems we need to move forward with the next iteration of the background controls (featured image, video URL etc) for the Cover block and then bring these background controls onward to other blocks.

At the same time it would be helpful to get a first iteration of the background controls to add an image into the Group block. As it has been a feature that many needed for a long time.

@critterverse
Copy link
Contributor

This is an awesome PR to see being worked on! This is such a long-standing request in Gutenberg that has been approached in a lot of different ways — I just wanted to weigh in on some of the things that have prevented us from moving forwad in the past.

One of my concerns is that Group now has both background and overlay. Cover only seems to have overlay.

The way that the overlay works in the Cover block has always been a bit confusing IMO, and prevents us from having parity with other blocks like the Group if we were to extend background capabilities there. A good first step towards unblocking this PR would be resolving how colors work in the Cover block. I personally would love to see further steps taken towards a true "layered" Color palette (more robust functionality around the way the Color panel works has been proposed a few times in the past, including in @javierarce's excellent post Interaction of Color). The layered panel could handle both background color and overlay color, with items at the top of the panel representing the foreground and items at the bottom of the panel representing the background like a traditional layers palette.

but one of the challenges is that the inspector controls for adding and managing backgrounds in the Cover block aren't yet that strong, and could use a great deal of refinement on their own

Also agree with what @jasmussen said above about the Cover block background controls needing a good deal of refinement. @javierarce and I shared some explorations around this recently in #20193 (comment)

Group opens up the additional question of what happens when the group is empty?
I'd say we leave it to do whatever it does naturally.

^ This is something that tripped me up when testing this PR. Because the Group block remains empty after adding media, it's difficult to select the Group block or get back to the toolbar in order to remove or replace the media.

As others have already mentioned, my instinct is that it might make more sense to refine the functionality of the Cover block to a point we’re happy with, and then look at expanding those features for the Group block (and potentially other blocks). Whatever road we take to get there, very happy to see background capabilites evolving!

@getdave
Copy link
Contributor Author

getdave commented Feb 15, 2022

Noting a counter argument here that the process of abstracting the commonalities before new background features are added to the Cover block might make it easier to have those features across both blocks.

Also by stress testing against more than one block we might be able to design better UI and UX as well as more flexible and resilient components.

Conversely, if folks feel overlay/background on the Cover isn't working well then let's not foist these problems on the Group block. Rather lets try and enhance the Cover block and then abstract to the Group block.

...in @javierarce's excellent post Interaction of Color)

Wow this is awesome. @critterverse @javierarce is there any work actively under way to implement any of these ideas? If not are there any concrete plans or roadmaps pointing in that direction?

@critterverse
Copy link
Contributor

critterverse commented Feb 15, 2022

is there any work actively under way to implement any of these ideas? If not are there any concrete plans or roadmaps pointing in that direction?

There's not a specific roadmap or timeline that I know of — but I know that some of us on the Design team, including @jasmussen and @javierarce, have done a lot of thinking about background tools and are excited to see further capabilities take shape! I think we're in a much better place to think about implementing some of these ideas now than we were even a year ago, based on the work that was done in #27331 — so it would be good to revisit the Cover block sometime soon.

I suggest that we link any design updates to this issue and #16479 to keep track of the conversation.

@ciampo
Copy link
Contributor

ciampo commented Feb 16, 2022

In other words, we know we need to improve the design controls for backgrounds, and it might be worth it to do that before we expand backgrounds to other blocks. That way, when we do we can potentially manage the complexity by making "Background" a hidden-by-default addition to various toolspanels.

One of my goals here would have been to refactor some of the tools so that they are resuable and shared between the
blocks. I assumed this would probably lead to them being picked up by someone working on Global Styles?

Probably good to involve folks who worked on the ToolsPanel component here (cc @aaronrobertshaw @andrewserong @ramonjd )

@getdave getdave added the [Type] Experimental Experimental feature or API. label Feb 16, 2022
@aaronrobertshaw
Copy link
Contributor

There was a discussion thread started recently that looked to explore the possibilities around background block support. If our intent is to be consistent across blocks, I'd suggest this might be the best mechanism to achieve that.

@aaronrobertshaw
Copy link
Contributor

As great as adding further background functionality to more blocks will be, I'd like to strongly recommend that we hold off on this.

Copying across the Cover block's functionality also copies its problems, introduces lots of new attributes, and modifies markup. All of which increases the complexity and maintenance burden. Managing deprecations as they currently are is a tricky and fraught process. We've only just recently seen issues and regressions with the Cover block around this area. @glendaviesnz and @ramonjd will be able to speak more on this.

My vote would be to approach this taking into account the fact we do want this functionality shared across a number of blocks and implement a consistent solution that can be opted into by each block requiring only the bare minimum of tweaks to the blocks themselves.

I appreciate the great desire and user demand for this but I do feel we can save a lot of headaches by pausing for a moment.

Interested to hear others' thoughts.

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.

Thanks for diving into this @getdave! I think it's a feature we'd all love to see in the group block.

Just echoing Aaron's linking to the discussion about a background block support — I think it's important that we take what we've learnt from the challenges in dealing with the complexity of the Cover block (and its many deprecations) and look into seeing if there's an abstracted background block support that we can build, instead of trying to duplicate logic between the two blocks. For example, a recent fix from @ramonjd for the Cover block involved updating 53 files, so it can be pretty tricky to work with!

At this stage I'm looking for feedback on feature parity with the Cover block.

So, rather than looking at feature parity between the Group block and the Cover block, I think it's a good time to ask, "What features of the Cover block are universal to any block where we might want a background?". There's a few other types of blocks that could also benefit from background images, including Column(s), Button, Media & Text. It'd be great to focus efforts on what will allow us to scale the support across any arbitrary block.

Port over save.js implementation from core/cover to allow for front end to reflect changes in editor.

Another thing to explore in developing a universal block support for background images, is to see how much of the logic we'd like to move to server-rendering, to both avoid the issue with deprecations, and also potentially to support dynamic values for background images (such as featured images), so it could also be a good opportunity to rethink how we save background image data.

Thanks again for pushing forward the discussion and opening the WIP!

@ramonjd
Copy link
Member

ramonjd commented Feb 17, 2022

Copying across the Cover block's functionality also copies its problems, introduces lots of new attributes, and modifies markup. All of which increases the complexity and maintenance burden. Managing deprecations as they currently are is a tricky and fraught process. We've only just recently seen issues and regressions with the Cover block around this area. @glendaviesnz and @ramonjd will be able to speak more on this.

Thanks for the ping.

I'm in agreement, if only to spare our future selves the trauma of juggling backwards compatibility and deprecations when things change.

Two recent examples are #38392 and #35065, where modest changes to the Cover Block sent us down a nightmarish vortex of fixtures, deprecations and test scenarios. I'm exaggerating for a laugh of course. Or am I? 😄

I like where this PR is going conceptually. It'll be fantastic to be able to offer theme authors the ability to customize block backgrounds.

However as mentioned, there are other blocks that might benefit from background controls, so it might be worth considering a block supports approach.

Something like that could even be extended to the theme/site level.

Cheers 🍺

@getdave
Copy link
Contributor Author

getdave commented Feb 18, 2022

Thanks for the feedback everyone. Exactly what I was hoping to stimulate with this PR 😄

I agree that we don't want to naively copy/paste between Cover and Group as we do in this PR. In the spirit of working incrementally I was hoping to use the Group as an exercise to determine which parts of "background images" are universal and which are block specific.

However, I will defer to your experiences with Cover which sound...challenging!

Question: are you aware of anyone currently working on this feature? As I expressed previously, creating an all-encompassing "backgrounds" solution sounds optimal but we might end up back here in 2years time still asking why the Group block doesn't support background images (I'm being facetious but you take my point).

Thanks again for all the great input. I appreciate your time 🙇

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @getdave,

Thank you for working on expanding background tools 👍

The cover block is complex and I think we should not try to port all its functionality to other blocks. If one needs something with advanced backgrounds probably the cover block is a right to block to use. If we port things in the cover block to the group block the group block may essentially become a cover block. The cover block is not just a background image, it supports things like setting a background video with an overlay gradient changing the colors in the video, that is not something done purely with CSS and requires specific markup to work. There were even tries in the past to allow having embeds as backgrounds e.g: a youtube video as background but as it required loading third-party scripts we did not pursue that.

I think having basic image backgrounds purely based on CSS may make sense in other blocks. In that case, I agree global styles and block supports seem the right place. Blocks would specify they support image backgrounds on block.json, and one would be able to change the image background at the block level and at the global styles level e.g: setting an image background for the site.
We would only support what CSS supports with simple rules e.g: setting an image background without overlays, videos etc...

As far as I know, no one is working on these tasks, and is something that can be worked on.

@andrewserong
Copy link
Contributor

Thanks @jorgefilipecosta for outlining the key differences between the cover block and how a background support might work. As far as I know work hasn't commenced on building a background support yet, but @glendaviesnz kicked off the discussion to see if folks have opinions on how it might work.

I like the idea that a background support would be restricted to a fairly simple CSS approach, and deferring the more complex use cases to the Cover block.

creating an all-encompassing "backgrounds" solution sounds optimal but we might end up back here in 2years time still asking why the Group block doesn't support background images

It is easy for desired features (like this one) to sit around for a long time without anyone working on it directly. The group block (and block supports in general) has received a lot of attention recently, but more in terms of layout supports and how we render the position and flow of blocks (and colors) rather than the background images in particular. Sounds like background support could be a good candidate for someone to pick up now since it seems like there's quite a bit of interest in seeing it added!

@paaljoachim
Copy link
Contributor

Thanks for adding in the discussion link Andrew!
I went ahead and added a bunch of issues to it to gather various pieces of the background puzzle.
#38119 (comment)

@getdave
Copy link
Contributor Author

getdave commented Feb 21, 2022

Thank you for working on expanding background tools 👍

No problem. The idea of this (draft) PR is to stimulate discussion around a much requested feature and to provide evidence of a requirement for the functionality to exist.

The cover block is complex and I think we should not try to port all its functionality to other blocks.

Agreed. Indeed I have omitted the "videos" functionality from this PR. There may be other things we would wish to omit also.

I think having basic image backgrounds purely based on CSS may make sense in other blocks. In that case, I agree global styles and block supports seem the right place. Blocks would specify they support image backgrounds on block.json, and one would be able to change the image background at the block level and at the global styles level e.g: setting an image background for the site.

I'm glad you say this because I don't think the Cover block is suitable in all circumstances. I'd like to see basic background image support added to the Group block. If other blocks can use it as well then so much the better. Let's explore that.

Screen Shot 2022-02-21 at 09 49 18

As per the 👍 on this PR, it looks like we have enough demand to warrant someone starting to work on this feature in a general (i.e. not block-specific) way as you describe.

@andrewserong
Copy link
Contributor

I've made a start exploring how a background image block support (that we could opt the Group block into) might work in a draft PR (#39243) — it's very early, but just thought I'd share that I'm looking into it since there's been so much interest in the feature. I'm hacking away at this around working on other things, so will continue chipping away at it as I have time, but I'm keen to explore implementing the feature. Happy for any feedback, of course!

@jasmussen
Copy link
Contributor

Wanted to mention a new ticket with a design for a single "Background" panel in #39427. It also features layers, but there are mockups for a more near term solution as well.

@getdave
Copy link
Contributor Author

getdave commented Mar 25, 2022

We won't pursue the route in this PR now so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block [Type] Enhancement A suggestion for improvement. [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants