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

Image: Add border block support for color, width, and style #31366

Merged
merged 21 commits into from
Jul 15, 2022

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Apr 30, 2021

Fixes: #38801

Depends on:

Part of:

Description

This takes advantage of border block support to add those features to the Image block.

  • Individual image border support added via block supports (skips serialization and manually applies to the inner img)
  • Leverages block support feature level selectors added in #42087 to allow theme.json and Global Styles theming of borders.
  • Theme.json can style the image borders via normal without having to use the additional nesting of the elements API

How has this been tested?

Fixture tests and manually.

Note: There is a current bug on trunk impacting resetting of controls.

Test Instructions

  1. Create a post, add an image block, set a border-radius on the image block and save the post
  2. Checkout this PR and reload the editor
  3. Ensure there are no block validation errors and the block still displays its border-radius correctly
  4. Set new border color, radius and width on the image block ensuring it updates correctly
  5. From the block controls toolbar, choose to crop the image
  6. Ensure the border still displays
  7. Cancel the cropping and save the post
  8. Confirm the image border displays correctly on the frontend
  9. Back in the editor add a gallery, with images. Apply different borders and captions to the images.
  10. After ensuring gallery images display ok, save the post and confirm for the frontend
  11. Try styling the image block via theme.json (see snippet below)
  12. Switch to the site editor and navigate within the Global Styles sidebar to Blocks > Image > Layout and test configuring borders on images.
  13. Run npm run test-unit and ensure the image fixture tests pass
{
	"styles": {
		"core/image": {
			"border": {
				"width": "15px",
				"style": "dashed",
				"color": "blue"
			}
		}
	}
}

Screenshots

Screen.Recording.2022-07-15.at.11.31.52.am.mp4

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 self-assigned this Apr 30, 2021
@aaronrobertshaw aaronrobertshaw added [Block] Image Affects the Image Block [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
@github-actions
Copy link

github-actions bot commented Apr 30, 2021

Size Change: +828 B (0%)

Total Size: 1.25 MB

Filename Size Change
build/block-editor/index.min.js 153 kB +33 B (0%)
build/block-library/blocks/gallery/style-rtl.css 1.53 kB +36 B (+2%)
build/block-library/blocks/gallery/style.css 1.53 kB +37 B (+2%)
build/block-library/blocks/image/editor-rtl.css 736 B -2 B (0%)
build/block-library/blocks/image/style-rtl.css 627 B +103 B (+20%) 🚨
build/block-library/blocks/image/style.css 630 B +100 B (+19%) ⚠️
build/block-library/editor-rtl.css 10.2 kB +5 B (0%)
build/block-library/editor.css 10.2 kB +5 B (0%)
build/block-library/index.min.js 184 kB +295 B (0%)
build/block-library/style-rtl.css 11.6 kB +108 B (+1%)
build/block-library/style.css 11.7 kB +108 B (+1%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 982 B
build/annotations/index.min.js 2.76 kB
build/api-fetch/index.min.js 2.26 kB
build/autop/index.min.js 2.14 kB
build/blob/index.min.js 475 B
build/block-directory/index.min.js 6.58 kB
build/block-directory/style-rtl.css 990 B
build/block-directory/style.css 991 B
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/style-rtl.css 14.6 kB
build/block-editor/style.css 14.6 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 103 B
build/block-library/blocks/audio/style.css 103 B
build/block-library/blocks/audio/theme-rtl.css 110 B
build/block-library/blocks/audio/theme.css 110 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 441 B
build/block-library/blocks/button/editor.css 441 B
build/block-library/blocks/button/style-rtl.css 543 B
build/block-library/blocks/button/style.css 543 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 187 B
build/block-library/blocks/comment-template/style.css 185 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/cover/editor-rtl.css 615 B
build/block-library/blocks/cover/editor.css 616 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 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 110 B
build/block-library/blocks/embed/theme.css 110 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 253 B
build/block-library/blocks/file/style.css 254 B
build/block-library/blocks/file/view.min.js 346 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 948 B
build/block-library/blocks/gallery/editor.css 950 B
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 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 327 B
build/block-library/blocks/html/editor.css 329 B
build/block-library/blocks/image/editor.css 737 B
build/block-library/blocks/image/theme-rtl.css 110 B
build/block-library/blocks/image/theme.css 110 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 705 B
build/block-library/blocks/navigation-link/editor.css 703 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 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation-submenu/view.min.js 423 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.96 kB
build/block-library/blocks/navigation/style.css 1.95 kB
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 443 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 96 B
build/block-library/blocks/post-comments-form/editor.css 96 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 632 B
build/block-library/blocks/post-comments/style.css 630 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 605 B
build/block-library/blocks/post-featured-image/editor.css 605 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 282 B
build/block-library/blocks/post-template/style.css 282 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 365 B
build/block-library/blocks/query/editor.css 364 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 385 B
build/block-library/blocks/search/style.css 386 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 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 464 B
build/block-library/blocks/shortcode/editor.css 464 B
build/block-library/blocks/site-logo/editor-rtl.css 708 B
build/block-library/blocks/site-logo/editor.css 708 B
build/block-library/blocks/site-logo/style-rtl.css 192 B
build/block-library/blocks/site-logo/style.css 192 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 322 B
build/block-library/blocks/spacer/editor.css 322 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 494 B
build/block-library/blocks/table/editor.css 494 B
build/block-library/blocks/table/style-rtl.css 611 B
build/block-library/blocks/table/style.css 609 B
build/block-library/blocks/table/theme-rtl.css 175 B
build/block-library/blocks/table/theme.css 175 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 178 B
build/block-library/blocks/template-part/editor.css 178 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 561 B
build/block-library/blocks/video/editor.css 563 B
build/block-library/blocks/video/style-rtl.css 159 B
build/block-library/blocks/video/style.css 159 B
build/block-library/blocks/video/theme-rtl.css 110 B
build/block-library/blocks/video/theme.css 110 B
build/block-library/common-rtl.css 987 B
build/block-library/common.css 984 B
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/theme-rtl.css 695 B
build/block-library/theme.css 700 B
build/block-serialization-default-parser/index.min.js 1.11 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 47 kB
build/components/index.min.js 230 kB
build/components/style-rtl.css 14 kB
build/components/style.css 14 kB
build/compose/index.min.js 11.7 kB
build/core-data/index.min.js 14.7 kB
build/customize-widgets/index.min.js 11.2 kB
build/customize-widgets/style-rtl.css 1.4 kB
build/customize-widgets/style.css 1.4 kB
build/data-controls/index.min.js 653 B
build/data/index.min.js 7.99 kB
build/date/index.min.js 32 kB
build/deprecated/index.min.js 507 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.69 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 4.02 kB
build/edit-navigation/style.css 4.03 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/index.min.js 30.5 kB
build/edit-post/style-rtl.css 6.97 kB
build/edit-post/style.css 6.97 kB
build/edit-site/index.min.js 53.3 kB
build/edit-site/style-rtl.css 8.23 kB
build/edit-site/style.css 8.22 kB
build/edit-widgets/index.min.js 16.5 kB
build/edit-widgets/style-rtl.css 4.35 kB
build/edit-widgets/style.css 4.35 kB
build/editor/index.min.js 39.4 kB
build/editor/style-rtl.css 3.65 kB
build/editor/style.css 3.65 kB
build/element/index.min.js 4.27 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 6.75 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.64 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.77 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.78 kB
build/keycodes/index.min.js 1.38 kB
build/list-reusable-blocks/index.min.js 1.74 kB
build/list-reusable-blocks/style-rtl.css 835 B
build/list-reusable-blocks/style.css 835 B
build/media-utils/index.min.js 2.93 kB
build/notices/index.min.js 953 B
build/nux/index.min.js 2.05 kB
build/nux/style-rtl.css 732 B
build/nux/style.css 728 B
build/plugins/index.min.js 1.94 kB
build/preferences-persistence/index.min.js 2.22 kB
build/preferences/index.min.js 1.3 kB
build/primitives/index.min.js 933 B
build/priority-queue/index.min.js 612 B
build/react-i18n/index.min.js 696 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.68 kB
build/reusable-blocks/index.min.js 2.22 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.53 kB
build/token-list/index.min.js 644 B
build/url/index.min.js 3.61 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 268 B
build/widgets/index.min.js 7.19 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

@paaljoachim
Copy link
Contributor

paaljoachim commented May 1, 2021

Testing by downloading the special Gutenberg plugin. Procedure:
https://make.wordpress.org/design/2021/03/03/testing-a-gutenberg-pull-request-pr/

I copied the TT1 experimental-theme.json file into Twenty Twenty One theme root folder. Renamed experimental-theme.json to theme.json, and replaced all the contents with:

"border": {
				"customColor": true,
				"customRadius": true,
				"customStyle": true,
				"customWidth": true
			}

Did a hard refresh. Even installed Autoptimize. Made another post. I have not been able to see the Border panel settings yet. I assume there is something I am not doing right.

@aaronrobertshaw
Copy link
Contributor Author

@paaljoachim thanks for testing this. It's working ok for me in the TwentyTwentyOne theme.

Renamed experimental-theme.json to theme.json, and replaced all the contents with:

I think the problem might be the vague testing instructions. I've updated them to include a link to the current theme.json file I last tested with.

If you did indeed replace all the experimental-theme.json contents with only the border config, the shape wouldn't be correct so the checks for whether the support is enabled and to show the inspector controls would fail.

Example experimental-theme.json with border block support enabled.

@aaronrobertshaw
Copy link
Contributor Author

Core Block Styles

In general, there is only the "Rounded" block style by default. This style could be removed after a further iteration to the border block support to allow a greater max border radius value.

TwentyTwentyOne Block Styles

The TwentyTwentyOne and TT1 Blocks themes also introduce "Borders" and "Frame" block styles. These styles could be achieved via the border block support (and padding for "Frame" style). The block styles do however give the user a simple, one-click option to set a consistent border.

Summary

If we were to remove the block styles, existing blocks with those styles would keep the is-style-<style> CSS class without the need for a deprecation. The class would appear under the Advanced CSS inspector control field.

Ultimately, I think whether we remove the block styles or not comes down to convenience and simplicity for the user at the price of some overlap and duplication between block styles and the block support features.

Happy to hear others' thoughts on this and/or iterate on the block styles in a separate PR.

@aaronrobertshaw aaronrobertshaw marked this pull request as ready for review May 4, 2021 04:23
Copy link
Member

@vdwijngaert vdwijngaert left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell! Works as expected and unit tests pass. Might need some more work when it comes to the block styles part. It could be confusing to have the width and color controls there when the border style is set to none (or default, depending on defaults), though.

But, as you said, but that can probably happen later down the line.

@aaronrobertshaw
Copy link
Contributor Author

Thanks for testing and the feedback.

Might need some more work when it comes to the block styles part. It could be confusing to have the width and color controls there when the border style is set to none (or default, depending on defaults), though.

I agree, this would be a nice improvement.

One difficulty I've previously run into in this area is there isn't a reliable means to retrieve what the "default" value of any global style is in the normal post editor. In the site editor these are available within the state. As more block support features are adopted, I think the need to address this will increase.

Also, there is an open issue to refine the border block support controls #31337. Part of the approach in that was to order the controls in line with the CSS order. Hiding and showing controls might not work well there. Either way, the actual block support controls will need to be refined in a separate PR as suggested.

@paaljoachim
Copy link
Contributor

paaljoachim commented May 5, 2021

Hey Aaron.

Thank you for working on this amazing PR! It is great to get Border settings in place!

Copied your experimental-theme.json file into Twenty Twenty One.

This is what I see. Brainstorming.

Border style drop down. Selecting "None" should hide the controls below.
I am wondering if "Default" and "None" both should have the Border controls hidden. (As already mentioned by @vdwijngaert ) Default could also be whatever Global styles have defined Image Border settings to be.

I noticed that when I move the slider a tooltip shows up, and will stick in place until I click somewhere else.

Screen Shot 2021-05-06 at 00 52 29

I went through and checked each Border style type - Solid, Dashed and Dotted. Backend and frontend looked identical.

Image Styles. These do not work correctly when Border affects are in place. Such as selecting a dotted frame with a Border width of 40, and Border radius of 30. Example clicking Rounded did not show a rounded image.

Screen Shot 2021-05-06 at 00 59 10

Clicking through Rounded (no affect), Borders (no affect) and Frame (made the image smaller).
This will gradually become a bigger issue for all blocks as more and more styling controls are added. Such as the Border, padding, margin and other controls. As specific Styles panel pre-created blocks will likely stop working as the user decides to instead use the specific custom controls a block offers. As there will be some clashes between these. There will likely come to a point where a user can style a block and save it as a style that is automatically added to the Styles panel. As it could then replace the default pre-created styles offered. (One would likely then also be able to click a pre-created style and see the various custom settings show the style through the controls.) Before that time there will just be some clashes between pre-created styles and the styles made by the user through the available controls.

I turned off all adjustments I have made through Border settings also the border color and now the rounded style worked again. The Rounded style is a bit glitchy when experimenting with the Border settings.

@aaronrobertshaw
Copy link
Contributor Author

Great feedback @paaljoachim! Thanks for taking the time to be so thorough.

I am wondering if "Default" and "None" both should have the Border controls hidden.

There is already an open issue to refine the border support UI. It will actually do away with the default and none options. The plan there is to use the border width to control the border display.

I'm actually in the process of working on the refinement of the border support UI. There are some blockers for it though which include:

  • supporting non-pixel units for border width and radius
  • implementing/adding new color and radius controls
  • splitting the border-radius attribute so individual corner radii can be controlled

There is also a second PR adding border support to the group block. Both that and this PR won't land before the border block support UI refinements are complete.

I noticed that when I move the slider a tooltip shows up, and will stick in place until I click somewhere else.

As part of the refinements mentioned above, the both the width and radius RangeControls are being replaced so hopefully this won't be an issue moving forward.

Image Styles. These do not work correctly when Border affects are in place. Such as selecting a dotted frame with a Border width of 40, and Border radius of 30. Example clicking Rounded did not show a rounded image.

Yes, the specificity of the border block support styles (inline styles) is greater than the CSS class styles applied via the block style e.g. Rounded.

In my earlier comment on the block styles I listed some options. We could remove the block style entirely but the block styles do offer a simple one click configuration option for users. Another possibility is upon changing the selected block style we could clear the border support attributes. That might be the best compromise option.

Frame (made the image smaller)

This is due to the Frame block style actually applying padding. Given the width of the block doesn't change its the image that shrinks in this case. All the border styles have been overridden via the deliberate selections in the border panel.

There will likely come to a point where a user can style a block and save it as a style that is automatically added to the Styles panel.
Before that time there will just be some clashes between pre-created styles and the styles made by the user through the available controls.

Agreed. Given themes can add their own block styles as well I'm not sure we can remove all possibility of conflict there. Having an agreed upon approach to handling it would certainly help. Especially given this will be the case for all block support provided styles not just borders.

@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 image blocks?

ImageBordersRebased

@jasmussen
Copy link
Contributor

This is what I see:

image border

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 image blocks?

I left a few comments on the Cover PR which also apply here. Things like setting a default border style and width if you start with a color, perhaps moving the border panel (if collapsed) to the bottom next to Advanced.

I also noticed that the "rounded" style no longer works for when a border radius is applied; I think this speaks to us needing to replace that style variation with a "preset shortcut". I.e. instead of applying is-style-rounded, now that border radius support is built in, clicking that thumbnail is just a shortcut to pre-configuring the border radius. This is likely a larger undertaking: style variations need a lot of love. But we need to think about what happens in the mean time, so both still work: which one takes precedence? Not entirely sure what the best interim solution here is, even if we know the longer term solution is a reimagining of the styles.

On a more high level, about all these border support PRs (thank you for your work!), here's my thinking about them:

  • Because they leverage the border support design by Jay, we are in decent place with the usability (Refine border style dropdown design #31333 and Refine border control panel with design tools #31337)
  • If we can fix the small flow issues (see comments on Cover PR), the flow will be addressed.
  • We’ll want to, in the future (probably after the new color swatch picker) revisit where border support lives. Just like how we are looking at unifying width/height tools in a unified Dimensions panel, I suspect there's an opportunity to rethink appearance related tools. We might end up back on a dedicated Border (or Stroke?) panel, but it's something to think about.
  • For this initial version, and especially since the panel is starting out collapsed, it seems like it's in a good place to launch for a few notable blocks. Probably not Paragraph, for example, but Pullquote and Cover seem good, probably even Group. This is considering the value these tools will bring to enabling patterns.

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the great feedback as always Joen!

Things like setting a default border style and width if you start with a color, perhaps moving the border panel (if collapsed) to the bottom next to Advanced.

I've replied in detail with my thoughts on the suggestions on the Cover block PR. Hopefully, they are clear enough.

Essentially, those thoughts boil down to our block supports being overrides for the merged style value coming from theme.json, global styles etc. We don't have access to those merged values in the block editor at present. Try to set any default on the block support provided controls would likely mean blindly overriding theme values the use might be happy with already.

I do appreciate the point being made though regarding the flow.

At the moment the simpler case is how it currently works which is consistent across block supports. It could also be the option that requires the least re-editing of values by the user depending on their theme.

I also noticed that the “rounded” style no longer works for when a border radius is applied; I think this speaks to us needing to replace that style variation with a “preset shortcut”.

Good observation. This has actually be spoken about and an issue created to track the discussion. The idea is to explore block style variations as sets of block support style attributes.

As you mentioned, the rounded block style is effectively just a shortcut to applying a standard border radius to the image

But we need to think about what happens in the mean time, so both still work: which one takes precedence?

In technical terms, the border block support can only apply the border radius via inline styles. It will take precedence short of !important being resorted to.

Conceptually, if a user is going to the lengths of expanding the border support panel and configuring an explicit border radius value, that makes sense to me in terms of taking precedence.

A rounded style that is kept consistent across images also sounds perfectly reasonable. The block style does offer that consistency until such a time as we have implemented styles as sets of style attributes.

On a more high level, about all these border support PRs (thank you for your work!), here’s my thinking about them:

If we can fix the small flow issues (see comments on Cover PR ), the flow will be addressed.

I’m not sure the suggested flow fixes work 100% with how block supports function. If we have to change how all block supports work, these will fast become large issues.

We’ll want to, in the future (probably after the new color swatch picker ) revisit where border support lives.

Agreed. That can be discussed and explored separately. We’ll be in a better place to do that once we have the new panel in place.

For this initial version, and especially since the panel is starting out collapsed, it seems like it’s in a good place to launch for a few notable blocks. Probably not Paragraph, for example, but Pullquote and Cover seem good, probably even Group.

Each block opting into the support will go require a PR to do so. That will provide the chance to evaluate them as required.

By the way, the group block already has been opted into border support. It’s only a matter of whether the theme enables those controls or not.

Thanks again for all the considered responses. It definitely helps me work out what I'm missing to progress these PRs.

@aaronrobertshaw aaronrobertshaw force-pushed the add/image-border-support branch from 48a8d1d to 8ed1174 Compare August 9, 2021 06:45
@aaronrobertshaw
Copy link
Contributor Author

I've rebased this PR and added some baseline border styles in line with the discussion on the Cover block border support PR. This also required adding the same baseline styles for the image crop area.

@aaronrobertshaw aaronrobertshaw force-pushed the add/image-border-support branch from ae570a3 to 24f4d0e Compare July 15, 2022 00:48
@aaronrobertshaw
Copy link
Contributor Author

@paaljoachim This PR is ready for further testing and hopefully a final review 🎉

@aaronrobertshaw aaronrobertshaw changed the title Image: Add new border block support for color and width Image: Add border block support for color, width, and style Jul 15, 2022
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

I've taken this for a run in the editor and it works as advertised for me!

✅ In the editor the block border css declarations appear on the img and .wp-block-image__crop-area elements. The border persists during and after cropping.
✅ Image borders for images and gallery images look good on the frontend
✅ Creating huge border widths in a gallery does not collapse columns
styles.blocks['core/image'].border styles are applied appropriately
✅ Global styles updated in the site editor are applied appropriately (and taken precedence over theme.json)

"radius": true,
"width": true,
"__experimentalSelector": "img, .wp-block-image__crop-area",
Copy link
Member

Choose a reason for hiding this comment

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

🍺

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.

Great work getting this to the finish line @aaronrobertshaw! Tested with a few different block and classic themes, and all working nicely 👍

✅ Crop tools are working nicely in the editor with and without a custom border
✅ Custom border color is working nicely in the editor and site editor
✅ Image border in global styles works well
✅ Deprecation changes look good to me visually

The only visual issue I encountered was with a caption on an image in a gallery block, where that image has a border and radius in global styles the Image block is set to have a border and radius. The linear gradient and absolute positioning of the figcaption mean that it extends over the area of the border radius:

image

Given the effort already to ensure that the border (and its radius) does not affect the figcaption, and that this is handled for individual blocks with radii, I think this isn't worth attempting to fix in this PR, particularly when separately there's work and recent PRs looking at customising the figcaption.

In the "tiny nits" department, elsewhere there's efforts to remove lodash as a dependency, so in a follow-up at some point, it might be worth us seeing if we can swap out the isEmpty calls for another approach — given the iterations on getting to this stage with this PR, though, definitely not a blocker for landing this one.

So, all in all this LGTM! 🎉

@aaronrobertshaw
Copy link
Contributor Author

I appreciate the reviews @ramonjd and @andrewserong, cheers 🍻

The only visual issue I encountered was with a caption on an image in a gallery block, where that image has a border and radius in global styles the Image block is set to have a border and radius. The linear gradient and absolute positioning of the figcaption mean that it extends over the area of the border-radius:

Thanks for raising this edge case.

When borders are applied individually to blocks, we can add an additional CSS class to them that allows us to adjust the positioning of the caption. When borders are configured via Global Styles they are targeted via a simple CSS selector and the block won't know whether or not the caption should be repositioned.

There could be a few options to explore in follow-ups.

  • As you note, if the caption becomes an inner block to the Image block, then it could provide styling options.
  • Additionally, the Gallery block could offer an option, or Block Style, to toggle all captions to overlay the image or not. This might provide better visual results in terms of consistency and only require a single update rather than across multiple blocks.

In the "tiny nits" department, elsewhere there's efforts to remove lodash as a dependency, so in a follow-up at some point, it might be worth us seeing if we can swap out the isEmpty calls for another approach

Agreed. This is a side effect of just how old and long-running this PR has been. I'll happily address this in a separate follow-up.

@aaronrobertshaw aaronrobertshaw merged commit c1d065f into trunk Jul 15, 2022
@aaronrobertshaw aaronrobertshaw deleted the add/image-border-support branch July 15, 2022 05:41
@github-actions github-actions bot added this to the Gutenberg 13.8 milestone Jul 15, 2022
@paaljoachim
Copy link
Contributor

Hi @aaronrobertshaw

Thank you very much for working on this feature Aaron! It is greatly appreciated!
I look forward to one day be able to save adjustments as a style and use it for all images.

Testing.
Lock and unlock icons. This is how I see it.
Lock icon = means all sides have the same width and color.
Unlock icon = means that width and/or color is not the same for all sides.

Clicking the Lock/Unlock icon will open or close the individual sides attributes/properties.

Example.
Here is a screenshot of Border where individual sides have their own property (likely wrong word to use) as in different color/width.
Radius is the same for all sides.

I would expect to see the Border icon show as unlocked because I have gone in and adjusted the properties of different sides.
But here we see that both icons are identical even though Border has individual side changes and Radius does not.

Screenshot 2022-07-15 at 15 55 25

@bph bph added Needs User Documentation Needs new user documentation Needs Dev Note Requires a developer note for a major WordPress release cycle labels Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Needs Dev Note Requires a developer note for a major WordPress release cycle Needs User Documentation Needs new user documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image Block: Border settings don't work when cropping/rotating