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

Cover: Add custom border support to the cover block #31370

Closed
wants to merge 6 commits into from

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Apr 30, 2021

Part of: #21540

Description

This takes advantage of the recently improved border block support (#30124) to add those features to the Cover block.

How has this been tested?

Manually

Test Setup

  1. Enable custom borders via your theme.json file. Under settings then either the defaults or core/cover context set the border feature flags below to true. Example json.
			"border": {
				"customColor": true,
				"customRadius": true,
				"customStyle": true,
				"customWidth": true
			}

Test Instructions

  1. Checkout this PR
  2. Create a post, add a cover block using solid background color
  3. Update the cover block with various border configurations and save the post
  4. Confirm the cover block displays correctly on the frontend
  5. Replace the colored background with an image
  6. Checked that dashed and dotted borders still display correctly on both the editor and frontend

Screenshots

CoverBorders

Types of changes

New feature.

Checklist:

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

@aaronrobertshaw aaronrobertshaw added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Apr 30, 2021
@aaronrobertshaw aaronrobertshaw self-assigned this Apr 30, 2021
@github-actions
Copy link

github-actions bot commented Apr 30, 2021

Size Change: +433 B (0%)

Total Size: 1.24 MB

Filename Size Change
build/block-library/blocks/cover/editor-rtl.css 620 B +74 B (+14%) ⚠️
build/block-library/blocks/cover/editor.css 620 B +73 B (+13%) ⚠️
build/block-library/blocks/cover/style-rtl.css 1.56 kB +32 B (+2%)
build/block-library/blocks/cover/style.css 1.56 kB +31 B (+2%)
build/block-library/editor-rtl.css 10.3 kB +40 B (0%)
build/block-library/editor.css 10.3 kB +40 B (0%)
build/block-library/index.min.js 181 kB +102 B (0%)
build/block-library/style-rtl.css 11.6 kB +21 B (0%)
build/block-library/style.css 11.6 kB +20 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
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.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 150 kB
build/block-editor/style-rtl.css 14.9 kB
build/block-editor/style.css 14.9 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/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 59 B
build/block-library/blocks/avatar/style.css 59 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-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 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-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 95 B
build/block-library/blocks/comments/editor.css 95 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 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 961 B
build/block-library/blocks/gallery/editor.css 964 B
build/block-library/blocks/gallery/style-rtl.css 1.51 kB
build/block-library/blocks/gallery/style.css 1.51 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 333 B
build/block-library/blocks/group/editor.css 333 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 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 463 B
build/block-library/blocks/latest-posts/style.css 462 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 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 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-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.95 kB
build/block-library/blocks/navigation/style.css 1.94 kB
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 395 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 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 260 B
build/block-library/blocks/paragraph/style.css 260 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 69 B
build/block-library/blocks/post-comments-form/editor.css 69 B
build/block-library/blocks/post-comments-form/style-rtl.css 495 B
build/block-library/blocks/post-comments-form/style.css 495 B
build/block-library/blocks/post-comments/editor-rtl.css 77 B
build/block-library/blocks/post-comments/editor.css 77 B
build/block-library/blocks/post-comments/style-rtl.css 628 B
build/block-library/blocks/post-comments/style.css 628 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 370 B
build/block-library/blocks/pullquote/style.css 370 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 369 B
build/block-library/blocks/query/editor.css 369 B
build/block-library/blocks/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 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 146 B
build/block-library/blocks/separator/editor.css 146 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 194 B
build/block-library/blocks/separator/theme.css 194 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 759 B
build/block-library/blocks/site-logo/editor.css 759 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 504 B
build/block-library/blocks/table/editor.css 504 B
build/block-library/blocks/table/style-rtl.css 625 B
build/block-library/blocks/table/style.css 625 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 149 B
build/block-library/blocks/template-part/editor.css 149 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 993 B
build/block-library/common.css 990 B
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/theme-rtl.css 689 B
build/block-library/theme.css 694 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 47 kB
build/components/index.min.js 227 kB
build/components/style-rtl.css 15 kB
build/components/style.css 15 kB
build/compose/index.min.js 11.7 kB
build/core-data/index.min.js 14.6 kB
build/customize-widgets/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/data/index.min.js 7.98 kB
build/date/index.min.js 32 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.69 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 4.05 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/index.min.js 30.4 kB
build/edit-post/style-rtl.css 7.08 kB
build/edit-post/style.css 7.08 kB
build/edit-site/index.min.js 47.6 kB
build/edit-site/style-rtl.css 7.96 kB
build/edit-site/style.css 7.95 kB
build/edit-widgets/index.min.js 16.4 kB
build/edit-widgets/style-rtl.css 4.41 kB
build/edit-widgets/style.css 4.4 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.3 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.1 kB
build/nux/style-rtl.css 751 B
build/nux/style.css 749 B
build/plugins/index.min.js 1.98 kB
build/preferences-persistence/index.min.js 2.23 kB
build/preferences/index.min.js 1.32 kB
build/primitives/index.min.js 949 B
build/priority-queue/index.min.js 628 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.2 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/vendors/react-dom.min.js 38.5 kB
build/vendors/react.min.js 4.34 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

@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review May 4, 2021 04:37
@jasmussen
Copy link
Contributor

I appreciate your PRs. But both Cover and Image (#31366) are very widely used, and this adds a pretty mammoth panel to the inspector for both. I think we need to land a PR to address #31337 before we add these panels.

@aaronrobertshaw aaronrobertshaw added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label May 5, 2021
@aaronrobertshaw
Copy link
Contributor Author

I appreciate your PRs. But both Cover and Image are very widely used, and this adds a pretty mammoth panel to the inspector for both. I think we need to land a PR to address #31337 before we add these panels.

Thanks for the feedback @jasmussen.

I'm working on a PR for #31337 at the moment though its not yet ready for review.

Screen Shot 2021-05-05 at 6 06 34 pm

There are going to be several delays with addressing that one. For example;

Any issues leaving this PR and #31366 open for the time being?

@jasmussen
Copy link
Contributor

Any issues leaving this PR and #31366 open for the time being?

Not at all! The feature is awesome and we want it. We just want to thread the needle of user experience and features.

@paaljoachim
Copy link
Contributor

I just tested this PR, and it works nicely. Looks good on the backend and frontend.
I look forward to testing the new approach.
Thanks!

@aaronrobertshaw
Copy link
Contributor Author

@jasmussen This PR has just been rebased to incorporate the recent border support UI changes. Do you feel this is in a place where we can move forward on adding border control to the cover block?

CoverBlockBorder

@jasmussen
Copy link
Contributor

I think it's close, and I think you've done excellent work. Here's what I see:

cover

A few things. If you choose a color, without first choosing a border style, no border is applied:

Screenshot 2021-07-09 at 09 09 27

We should probably either default to the solid border style, or apply it as soon as you set a border width.

In https://github.com/WordPress/gutenberg/pull/31370/files#diff-9e449d4b8bd69c9771de8e20c9a1d78798a6934f99e3bfc432df6ceb23def7f4R14 you apply an overflow: hidden; to enable border radius. While I think border-radius is very compelling, I think we need to find a different way to accomplish it, as overflow: hidden; crops the resize handle:

Screenshot 2021-07-09 at 09 09 15

If I don't choose any color for the border, I'm pretty sure that the border will be the same color as the text, (i.e. currentColor). But it's worth testing in a theme with a black background and white text, just to be sure, and if not, just explicitly set currentColor as the default color.

Question: should we set a border-width of at least 1px if you choose a color before choosing a border width? Just so a border is actually visible if you start there? This is what Figma does:

figma

Another question: should we move the border panel, since it's collapsed by default, to the bottom of the inspector? As noted across a few PRs, I think we want to revisit where border tools live in future iterations. But since it'll launch collapsed by default, it might make sense to group it at the bottom with the other collapsed "Advanced" panel.

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the thoughtful feedback @jasmussen.

We should probably either default to the solid border style, or apply it as soon as you set a border width.

I'm not convinced we should do either of those just yet but that could be that we aren't quite on the same page with what block supports feature do and provide at this level. Hopefully my understanding of this will be clearer through the responses to your questions below.

Currently, not applying any value in a field provided by a block support feature means the block can inherit values via the stylesheet generated by theme.json or global styles. This is how other block supports such as the padding and margin features inherit values from the theme or global styles. Unfortunately, at this point in time we do not have access to merged theme, global and user styles within the block editor.

As I understand it, the aim of these block support features/styles at the individual block level is to give the user a means by which to override theme or global styles.

Regarding applying a style as soon as a color is selected. It would be possible. The catch there though is not knowing whether or not a style was set via the theme or global styles and this change then overriding that when the user didn't actually ask for it.

I currently have a PR nearly ready to land that will set the border style to none when the user sets the border width to 0. That could be considered a precedent to changing multiple values at once. I see that as a different situation though as a border width of 0 is essentially setting no border and is what the user is asking for.

If I don’t choose any color for the border, I’m pretty sure that the border will be the same color as the text, (i.e. currentColor).

This would depend on the theme. It may set the border color via the border.color style in theme.json or via its CSS.

But it’s worth testing in a theme with a black background and white text, just to be sure, and if not, just explicitly set currentColor as the default color.

This would mean overriding whatever the theme had set already elsewhere. I don't believe we can set defaults that don't match the theme for this reason. Without access to the merged theme/global styles, we could only hope to extract computed styles from DOM elements. Given ad hoc application of block support provided styles for some blocks, accessing computed styles would be hit and miss.

I think generic styles or defaults are better set at the theme.json level.

Question: should we set a border-width of at least 1px if you choose a color before choosing a border width? Just so a border is actually visible if you start there? This is what Figma does:

Automatically changing the width when setting a color would be another case of overriding the theme value when the user might not intend or want to.

Take the example where a theme sets a 3px black border. I just want to change the color to white. In the suggested scenario, I select the white color and it changes the width on me to 1px since the width field was previously blank. The possible inconvenience could be doubled because now as the user without inspecting the block with dev tools I likely don't know what the border width was and need to fiddle with it to get it back consistent with other blocks.

Your Figma example is a little different. In that case, you are adding a new border. Something that doesn't exist yet. So default values make sense, they aren't trying to blindly inherit from a theme or global styles. In the case of border support controls we are attempting to only add overrides to values that might already exist and have values defined elsewhere.

Another question: should we move the border panel, since it’s collapsed by default, to the bottom of the inspector?

All block support provided panels come from the same place in the code. Any change to the order will be reflected across all blocks. In the past, I believe there was discussion around the sidebar and ordering the panels alphabetically.

I'm not opposed to reordering the block support provided panels however that should be a separate PR to this. The impacts any change in order have on all blocks can be discussed on that PR within context. I also don't think it should hold up adding border support to the cover block.

@jasmussen
Copy link
Contributor

Thanks for the thorough response and sorry for the AFK-belated response.

Regarding the "setting a border style by default" question, my angle on this is purely a user experience one, where it feels broken when setting a width and not seeing any result. So I'm not trying to challenge any approaches to the global styles structure, that seems on a strong path, only how we can handle it from the surface/experience layer.

Regarding applying a style as soon as a color is selected. It would be possible. The catch there though is not knowing whether or not a style was set via the theme or global styles and this change then overriding that when the user didn't actually ask for it.

  • Can we provide a default border style, something like border-style: solid; border-width: 0;? With low enough specificity it should make for a default that isn't "nothing".
  • Can we chain the UX controls so that setting a width also sets a style?
  • Can the width box be grayed until a style is chosen?

I'm sure there are all sorts of challenges with these suggestions, but it can hopefully inspire some better ideas.

This would depend on the theme. It may set the border color via the border.color style in theme.json or via its CSS.
This would mean overriding whatever the theme had set already elsewhere. I don't believe we can set defaults that don't match the theme for this reason. Without access to the merged theme/global styles, we could only hope to extract computed styles from DOM elements. Given ad hoc application of block support provided styles for some blocks, accessing computed styles would be hit and miss.

Right, if the theme sets the color, that should be the color. I'm referring to the cases, which will likely be the majority of cases, where the theme does not set the color, we should fall back to currentColor if at all possible, so that we have good defaults.

To an extent that seems to be what it all boils down to, defaults and baselines:

  1. If a theme does not set any specific properties in theme.json, we need a way to set defaults.
  2. If a theme sets properties using theme.json, those should be used.
  3. If a user sets properties, they need to easily override both 1 and 2.

To elaborate on 1, I mean CSS defaults. Just like how if you do not provide a stylesheet on a webpage, you get Times New Roman in black text on a white background, we need to provide CSS default styles for blocks.

This is where I can see us leveraging :where for providing better defaults than the browser provides. This codepen illustrates how we can provide default colors that are easily overridden by either global styles or users. Essentially:

html :where(.has-red-background-color) {
	background-color: maroon;
}

That sets the default value of the "red" color to maroon.

.has-red-background-color {
	background-color: red;
}

With zero added specificity, the above selector can then update that value to red instead.

It's using this CSS addition to the particular block or blocks or design tool in question, that I'd like to see if we could add defaults. For example:

html :where(.wp-block-cover) {
	border-style: solid;
	border-width: 0;
	border-color: currentColor;
}

☝️ the border-color should already behave that way, i.e. if you specifiy border: 1px solid; it's basically the same as border: 1px solid currentColor;, however if necessary it could still be added. currentColor would then be the same as the color defined on the Cover block.

CSS-tricks has a bit more on using :where to provide baselines: https://css-tricks.com/using-the-specificity-of-where-as-a-css-reset/

@aaronrobertshaw
Copy link
Contributor Author

Thanks for taking the time to go into such detail @jasmussen, I definitely appreciate it. 👍

I've added a baseline style for a cover block's border.

html :where(.wp-block-cover) {
	border: 0 solid currentColor;
}

As you promised, it provides a means for the user to be more likely to get immediate visual feedback when editing individual border properties.

This doesn't address the issue raised where if a border color is chosen before a width, the user doesn't see a border, unless the theme sets a border width elsewhere by default.

Given we can't easily determine exact defaults or merged border styles and then reflect that within the block support provided controls, I think this is as close as we can get to the optimal experience.

Let me know if there is anything further we need to tweak to see this one merged.

@jasmussen
Copy link
Contributor

Thanks for trying it. As you note, it's only a small improvement, as you still need a width. And if we do add a >0 width, you get this:

Screenshot 2021-08-09 at 09 33 12

So it seems like the best way experience might be for a border-width to be explicitly set at the same time as selecting a border-style. Which primarily, as I understand it, opens a technical question, so I'd love thoughts on how arduous this would be to accomplish, and then we can go from there. And we might even decide: it's fine — you have to choose a width before you see anything.

One issue we have to solve, though, is the cropped resize handle. Are there any alternatives to applying overflow: hidden; to the container?

Screenshot 2021-08-09 at 09 35 56

@aaronrobertshaw
Copy link
Contributor Author

So it seems like the best way experience might be for a border-width to be explicitly set at the same time as selecting a border-style. Which primarily, as I understand it, opens a technical question, so I'd love thoughts on how arduous this would be to accomplish, and then we can go from there.

Currently, we do not have access to the merged theme and global styles in the block editor. If we accept that, another possibility I explored was to try and determine the computed border-width style for the block.

Given this needs to be done via block supports and it must handle blocks generically, I don't believe we can correctly access the computed style. Some blocks will skip serialization on the main block wrapper and apply it elsewhere or adjust the value. This means the block support can't determine which element to retrieve computed styles for.

As I see it we are left facing the override problem described earlier.

We can force a border width when a border style is selected. That part is easy. The issue is it comes at the cost of accepting that we are blindly overriding any border width configured by the theme.json or global styles.

If the user wants what was set by the theme or they themselves set in the global styles, they'll need to dig up that value and apply it manually. They'll also then need to update the block any time the theme or global styles changed. This might not be a common use case but it highlights that applying any width value at the block support level is done in absence of knowing what's really being displayed.

Overriding what is set by theme.json and global styles might be acceptable. Hopefully, through this discussion, we're making an informed decision on what taking that position entails.

One issue we have to solve, though, is the cropped resize handle. Are there any alternatives to applying overflow: hidden; to the container?

My apologies. That one had slipped my mind.

After a quick play with the styles, I believe we can apply a border-radius: inherit style on the inner :before element. While testing that however, I found that the introduction of border widths interferes with the resizable box height calculations. That will take me a little more time and testing to sort out but I will address that in the next couple of days.

Thanks for shining the light back on this issue 👍

@ramonjd
Copy link
Member

ramonjd commented Aug 16, 2021

Hi folks!

I raised the question of what to do about the border styles not appearing until a border style is selected over at #34061 (comment) (setting border control defaults in the tool panel).

The scenario is where we don't show border style by default in the ToolsPanel.

With things the way they are, I think having the styles hidden might exacerbate the UX challenge we have here, namely, folks wondering why the default controls have no effect with no apparent way out (unless they fiddle around with the default controls).

Strictly speaking about the editor, I can empathise with users who might not know that they have to turn on border style to see any border effects.

I see there are concerns in relation to styles overwriting defined higher up the chain however.

Are there any other considerations to bear in mind should we decide to set a default style in the border controls?

Overriding what is set by theme.json and global styles might be acceptable.

Isn't the case that styles set in editor controls such as border will override global styles anyway?

Would we need to communicate that in the editor, or do we expect editors to know this?

Maybe it's a matter of openly communicating this somewhere in the workspace.

It also occurred to me to check the computed styles as a general way of testing existing styles. Not sure how to get around blocks that skip serialization unless we know the target element.

I'm ready to help implement something to mitigate this, but just want to make sure I understand the balancing act correctly 😄

Thank you!

@aaronrobertshaw
Copy link
Contributor Author

Isn't the case that styles set in editor controls such as border will override global styles anyway?

Yes, adjusting the block support controls deliberately sets whatever the user asks for. The difference here is when adjusting the width the user isn't directly asking for the style to change.

Would we need to communicate that in the editor, or do we expect editors to know this?

My understanding is that we expect users to know if they change something for an individual block that overrides more general settings applied via global styles or theme.json. Again, this is slightly different from them adjusting the width and us making the assumption the style must change as well. We can do it. It would be good to confirm the trade-offs are acceptable first.

Maybe it's a matter of openly communicating this somewhere in the workspace.

I'm not sure how we can communicate this concisely and effectively within the sidebar. It might be worth exploring if there are possibilities where the border width and style controls could be combined, similar to how the "font appearance" control combines both font weight and style e.g. thin italic etc.

It also occurred to me to check the computed styles as a general way of testing existing styles. Not sure how to get around blocks that skip serialization unless we know the target element.

We could attempt to find the computed style of the block itself and use that for all blocks that don't skip the serialization of the block support. It doesn't solve the problem but does help mitigate the UX issue. Combine this with per block styles that apply some default, easily overridden border style via :where or something in CSS and perhaps this edge case becomes manageable.

@paaljoachim
Copy link
Contributor

Can we get a status update on this PR?
Thank you!

@aaronrobertshaw
Copy link
Contributor Author

Can we get a status update on this PR?

We are currently waiting for the new border controls to land.

There are two PRs adding refined border control components and a third that uses these to improve border support to allow individual side borders. Once they land we will be better positioned to roll out border support across blocks more widely.

@aaronrobertshaw
Copy link
Contributor Author

This PR has been switched back to draft status until some issues within the editor can be worked through.

I've trialled two new approaches to try and align the editor with the frontend when borders are supported on the Cover block.

  1. 783bcff was looking good until I realised that in the editor borders overlayed the content whereas on the frontend the content is contained within the border.
  2. cba150a is probably the closest I've gotten to aligning editor and frontend. The catch is it wraps the Cover block and a resizable box in a new div. I'm not sure this will be acceptable.

I've left this PR on the second option for the moment and will continue iterating on this next week.

Below demonstrates the current state of the Cover block in this PR.

Screen.Recording.2022-04-29.at.5.10.25.pm.mp4

@glendaviesnz
Copy link
Contributor

Approach number 2 didn't work for me in the editor with the default black overlay, but adding the following to the editor.scss fixed it:

.wp-block-cover__background.has-background-dim:not([class*=-background-color]) {
    background-color: #000;
}

I have gone around in circles all day trying various things, but no joy! You can almost use ResizeableBox as the block wrapper div if you change it from self closing to a parent div with rest of block as children, but it breaks a few things as the blockprops ref doesn't link being passed to that functional component.

This is a really frustrating one, as apart from the blue resize outlines being a bit cutoff everything works nicely with a very simple addition of the border supports and an overflow: hidden on the block wrapper. Will keep pondering it.

@aaronrobertshaw
Copy link
Contributor Author

After some further experimentation I've put up a separate PR exploring an alternate approach to getting the Cover block to behave in the editor with borders. It isn't perfect either unfortunately.

More info in #40774.

In terms of where we are at with adopting border support for Cover blocks, we have some options each with their own problems.


Option 1: Simple opt-in and accept UX issues

Pros
  • Simple
  • No new markup in editor
Cons
  • Drag handles are cut off
  • Resizing is jumpy due to height not taking into account border widths (if it was only block specific borders we might be able to overcome this. Once you introduce global styles/theme.json we don't have access to it those values yet)

Option 2: Refactor editor markup to wrap block in new div, make ResizableBox sibling to the block (cba150a)

Pros
  • Resizing isn't jumpy
  • Drag handles aren't clipped
Cons
  • Alters markup in the editor
    • ResizableBox also creates a difference between editor and frontend anyway
    • This PR attempts to minimize it by maintaining most classes and props on the new wrapper (minus the `wp-block-cover)
  • Needs some further testing with theme.json/global styles etc

Option 3: Add wrapper within Cover block in editor

Pros
  • Probably provides the cleanest UX (ignoring theme.json/global styles)
  • No clipping of resize drag handles
  • The change in markup has slightly less chance to break things in terms general layout of blocks in the editor (compared with Option 2)
Cons
  • The new wrapper needs to be absolutely positioned to avoid interference from padding theme.json or global styles
  • Borders applied via theme.json or global styles will not display correctly
    • This is already an issue on some other blocks e.g. Search block see #32417

Option 4: Something else

Open to any ideas on this one 🙏

These new styles have zero specificity and are easily overridden by theme or global styles. They however ensure there is some value set such that a user is more likely to be given immediate visual feedback when editing individual border properties.
This still isn't perfect. The cover content/background appears to be "under" dashed/dotted borders in the editor but are instead contained within them on the frontend.
@aaronrobertshaw
Copy link
Contributor Author

Option 4: Something else

Another option is to move the ResizableBox that is causing issues into a BlockPopover. A draft PR is up in #41153 though it needs some further work for the placeholder view but is looking promising.

@aaronrobertshaw
Copy link
Contributor Author

Closing this in favour of bringing #41153 up-to-date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Status] Blocked Used to indicate that a current effort isn't able to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants