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

Add support for column and row span in grid children. #58539

Merged
merged 26 commits into from
Feb 14, 2024

Conversation

tellthemachines
Copy link
Contributor

What?

Starts addressing #57478.

Adds child layout controls for grid children. Parent blocks have to opt in to this behaviour by adding "allowSizingOnChildren": true in their layout config.

The UI so far is just a Column span and a Row span inputs under Dimensions. They take a number, which is translated into grid-column: span [columnSpan]. Because this is meant to work in the current auto-columns default mode of grid layout, there's no upper limit to the number of columns allowed.

In the future, if we want to add the ability to pick a specific column/row to place the block, we can add, say, a columnStart attribute and change the declaration to grid-column: [columnStart] / span [columnSpan]

Testing Instructions

  1. Enable the Grid Group variation in the Gutenberg > Experiments admin page.
  2. Add a Grid to a post or template, throw in a few child blocks, and play with the controls under Dimensions in the child blocks sidebar.

One known limitation of this approach combined with the auto columns behaviour is that, if we define a span of 3 columns for a grid child and shrink the screen down to a size where less than 3 columns total fit, the grid will keep the 3 columns and overflow the viewport if needed. There's no smart way of detecting the column number resizing accordingly... yet 😇

Testing Instructions for Keyboard

Screenshots or screencast

Screenshot 2024-02-01 at 4 53 59 pm

@tellthemachines tellthemachines added [Type] Enhancement A suggestion for improvement. [Feature] Layout Layout block support, its UI controls, and style output. labels Feb 1, 2024
@tellthemachines tellthemachines self-assigned this Feb 1, 2024
@tellthemachines tellthemachines added Needs Dev Note Requires a developer note for a major WordPress release cycle Needs PHP backport Needs PHP backport to Core labels Feb 1, 2024
Copy link

github-actions bot commented Feb 1, 2024

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/block-supports/layout.php
❔ lib/experimental/kses.php
❔ phpunit/block-supports/layout-test.php

Copy link

github-actions bot commented Feb 1, 2024

Size Change: +369 B (0%)

Total Size: 1.71 MB

Filename Size Change
build/block-editor/index.min.js 251 kB +369 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 955 B
build/annotations/index.min.js 2.69 kB
build/api-fetch/index.min.js 2.32 kB
build/autop/index.min.js 2.1 kB
build/blob/index.min.js 578 B
build/block-directory/index.min.js 7.22 kB
build/block-directory/style-rtl.css 1.02 kB
build/block-directory/style.css 1.02 kB
build/block-editor/content-rtl.css 4.35 kB
build/block-editor/content.css 4.35 kB
build/block-editor/default-editor-styles-rtl.css 381 B
build/block-editor/default-editor-styles.css 381 B
build/block-editor/style-rtl.css 15.4 kB
build/block-editor/style.css 15.3 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 126 B
build/block-library/blocks/audio/theme.css 126 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 104 B
build/block-library/blocks/avatar/style.css 104 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 415 B
build/block-library/blocks/button/editor.css 414 B
build/block-library/blocks/button/style-rtl.css 627 B
build/block-library/blocks/button/style.css 626 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 421 B
build/block-library/blocks/columns/style.css 421 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 647 B
build/block-library/blocks/cover/editor.css 650 B
build/block-library/blocks/cover/style-rtl.css 1.69 kB
build/block-library/blocks/cover/style.css 1.68 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 98 B
build/block-library/blocks/details/style.css 98 B
build/block-library/blocks/embed/editor-rtl.css 322 B
build/block-library/blocks/embed/editor.css 322 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 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 316 B
build/block-library/blocks/file/editor.css 316 B
build/block-library/blocks/file/style-rtl.css 280 B
build/block-library/blocks/file/style.css 281 B
build/block-library/blocks/file/view.min.js 324 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/form-input/editor-rtl.css 227 B
build/block-library/blocks/form-input/editor.css 227 B
build/block-library/blocks/form-input/style-rtl.css 343 B
build/block-library/blocks/form-input/style.css 343 B
build/block-library/blocks/form-submission-notification/editor-rtl.css 340 B
build/block-library/blocks/form-submission-notification/editor.css 340 B
build/block-library/blocks/form-submit-button/style-rtl.css 69 B
build/block-library/blocks/form-submit-button/style.css 69 B
build/block-library/blocks/form/view.min.js 471 B
build/block-library/blocks/freeform/editor-rtl.css 2.61 kB
build/block-library/blocks/freeform/editor.css 2.61 kB
build/block-library/blocks/gallery/editor-rtl.css 947 B
build/block-library/blocks/gallery/editor.css 952 B
build/block-library/blocks/gallery/style-rtl.css 1.72 kB
build/block-library/blocks/gallery/style.css 1.72 kB
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 654 B
build/block-library/blocks/group/editor.css 654 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 189 B
build/block-library/blocks/heading/style.css 189 B
build/block-library/blocks/html/editor-rtl.css 336 B
build/block-library/blocks/html/editor.css 337 B
build/block-library/blocks/image/editor-rtl.css 863 B
build/block-library/blocks/image/editor.css 862 B
build/block-library/blocks/image/style-rtl.css 1.6 kB
build/block-library/blocks/image/style.css 1.59 kB
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/image/view.min.js 1.54 kB
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 505 B
build/block-library/blocks/media-text/style.css 503 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 668 B
build/block-library/blocks/navigation-link/editor.css 669 B
build/block-library/blocks/navigation-link/style-rtl.css 259 B
build/block-library/blocks/navigation-link/style.css 257 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/editor-rtl.css 2.25 kB
build/block-library/blocks/navigation/editor.css 2.26 kB
build/block-library/blocks/navigation/style-rtl.css 2.24 kB
build/block-library/blocks/navigation/style.css 2.23 kB
build/block-library/blocks/navigation/view.min.js 1.1 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 377 B
build/block-library/blocks/page-list/editor.css 377 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 235 B
build/block-library/blocks/paragraph/editor.css 235 B
build/block-library/blocks/paragraph/style-rtl.css 335 B
build/block-library/blocks/paragraph/style.css 335 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 508 B
build/block-library/blocks/post-comments-form/style.css 508 B
build/block-library/blocks/post-content/editor-rtl.css 74 B
build/block-library/blocks/post-content/editor.css 74 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 666 B
build/block-library/blocks/post-featured-image/editor.css 662 B
build/block-library/blocks/post-featured-image/style-rtl.css 342 B
build/block-library/blocks/post-featured-image/style.css 342 B
build/block-library/blocks/post-navigation-link/style-rtl.css 215 B
build/block-library/blocks/post-navigation-link/style.css 214 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 409 B
build/block-library/blocks/post-template/style.css 408 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 125 B
build/block-library/blocks/preformatted/style.css 125 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 354 B
build/block-library/blocks/pullquote/style.css 354 B
build/block-library/blocks/pullquote/theme-rtl.css 168 B
build/block-library/blocks/pullquote/theme.css 168 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 288 B
build/block-library/blocks/query-pagination/style.css 284 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 486 B
build/block-library/blocks/query/editor.css 486 B
build/block-library/blocks/query/view.min.js 958 B
build/block-library/blocks/quote/style-rtl.css 237 B
build/block-library/blocks/quote/style.css 237 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 140 B
build/block-library/blocks/read-more/style.css 140 B
build/block-library/blocks/rss/editor-rtl.css 149 B
build/block-library/blocks/rss/editor.css 149 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 184 B
build/block-library/blocks/search/editor.css 184 B
build/block-library/blocks/search/style-rtl.css 614 B
build/block-library/blocks/search/style.css 614 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/search/view.min.js 478 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 229 B
build/block-library/blocks/separator/style.css 229 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 323 B
build/block-library/blocks/shortcode/editor.css 323 B
build/block-library/blocks/site-logo/editor-rtl.css 754 B
build/block-library/blocks/site-logo/editor.css 754 B
build/block-library/blocks/site-logo/style-rtl.css 204 B
build/block-library/blocks/site-logo/style.css 204 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 682 B
build/block-library/blocks/social-links/editor.css 681 B
build/block-library/blocks/social-links/style-rtl.css 1.49 kB
build/block-library/blocks/social-links/style.css 1.48 kB
build/block-library/blocks/spacer/editor-rtl.css 350 B
build/block-library/blocks/spacer/editor.css 350 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 395 B
build/block-library/blocks/table/editor.css 395 B
build/block-library/blocks/table/style-rtl.css 639 B
build/block-library/blocks/table/style.css 639 B
build/block-library/blocks/table/theme-rtl.css 146 B
build/block-library/blocks/table/theme.css 146 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 403 B
build/block-library/blocks/template-part/editor.css 403 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/term-description/style-rtl.css 111 B
build/block-library/blocks/term-description/style.css 111 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 185 B
build/block-library/blocks/video/style.css 185 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.1 kB
build/block-library/common.css 1.1 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 12.3 kB
build/block-library/editor.css 12.3 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 216 kB
build/block-library/reset-rtl.css 472 B
build/block-library/reset.css 472 B
build/block-library/style-rtl.css 14.8 kB
build/block-library/style.css 14.7 kB
build/block-library/theme-rtl.css 688 B
build/block-library/theme.css 693 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/blocks/index.min.js 51.6 kB
build/commands/index.min.js 15.6 kB
build/commands/style-rtl.css 921 B
build/commands/style.css 918 B
build/components/index.min.js 226 kB
build/components/style-rtl.css 11.8 kB
build/components/style.css 11.8 kB
build/compose/index.min.js 12.6 kB
build/core-commands/index.min.js 2.77 kB
build/core-data/index.min.js 72.7 kB
build/customize-widgets/index.min.js 12.1 kB
build/customize-widgets/style-rtl.css 1.32 kB
build/customize-widgets/style.css 1.32 kB
build/data-controls/index.min.js 640 B
build/data/index.min.js 8.93 kB
build/date/index.min.js 17.9 kB
build/deprecated/index.min.js 451 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.65 kB
build/edit-post/classic-rtl.css 544 B
build/edit-post/classic.css 545 B
build/edit-post/index.min.js 25.1 kB
build/edit-post/style-rtl.css 5.63 kB
build/edit-post/style.css 5.62 kB
build/edit-site/index.min.js 212 kB
build/edit-site/style-rtl.css 15.3 kB
build/edit-site/style.css 15.3 kB
build/edit-widgets/index.min.js 17.3 kB
build/edit-widgets/style-rtl.css 4.21 kB
build/edit-widgets/style.css 4.21 kB
build/editor/index.min.js 61.8 kB
build/editor/style-rtl.css 5.43 kB
build/editor/style.css 5.43 kB
build/element/index.min.js 4.83 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 7.89 kB
build/format-library/style-rtl.css 478 B
build/format-library/style.css 477 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.58 kB
build/interactivity/file.min.js 447 B
build/interactivity/image.min.js 1.67 kB
build/interactivity/index.min.js 12.7 kB
build/interactivity/navigation.min.js 1.24 kB
build/interactivity/query.min.js 740 B
build/interactivity/router.min.js 1.29 kB
build/interactivity/search.min.js 618 B
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.74 kB
build/keycodes/index.min.js 1.46 kB
build/list-reusable-blocks/index.min.js 2.11 kB
build/list-reusable-blocks/style-rtl.css 836 B
build/list-reusable-blocks/style.css 836 B
build/media-utils/index.min.js 2.9 kB
build/modules/importmap-polyfill.min.js 12.2 kB
build/notices/index.min.js 948 B
build/nux/index.min.js 2 kB
build/nux/style-rtl.css 735 B
build/nux/style.css 732 B
build/patterns/index.min.js 5.79 kB
build/patterns/style-rtl.css 540 B
build/patterns/style.css 539 B
build/plugins/index.min.js 1.8 kB
build/preferences-persistence/index.min.js 2.05 kB
build/preferences/index.min.js 2.82 kB
build/preferences/style-rtl.css 698 B
build/preferences/style.css 700 B
build/primitives/index.min.js 975 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 1 kB
build/react-i18n/index.min.js 623 B
build/react-refresh-entry/index.min.js 9.47 kB
build/react-refresh-runtime/index.min.js 6.78 kB
build/redux-routine/index.min.js 2.7 kB
build/reusable-blocks/index.min.js 2.72 kB
build/reusable-blocks/style-rtl.css 243 B
build/reusable-blocks/style.css 243 B
build/rich-text/index.min.js 10.4 kB
build/router/index.min.js 1.79 kB
build/server-side-render/index.min.js 1.95 kB
build/shortcode/index.min.js 1.39 kB
build/style-engine/index.min.js 2.08 kB
build/token-list/index.min.js 582 B
build/url/index.min.js 3.72 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 957 B
build/warning/index.min.js 249 B
build/widgets/index.min.js 7.21 kB
build/widgets/style-rtl.css 1.15 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.02 kB

compressed-size-action

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'm having a lot of fun with this so far! It's working great for me in Chrome/FF/Safari.

Screenshot 2024-02-02 at 9 28 29 am

I don't know if it's in scope, or even a good idea, but I was wondering about a stack option for mobile, similar to what the Columns block does. Should Gutenberg offer this for grid too, or is it rather a theme responsibility?

Copy link

github-actions bot commented Feb 1, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: noisysocks <noisysocks@git.wordpress.org>
Co-authored-by: SaxonF <saxonafletcher@git.wordpress.org>
Co-authored-by: aristath <aristath@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@tellthemachines
Copy link
Contributor Author

Thanks for testing @ramonjd !

I was wondering about a stack option for mobile

If you're using the default Grid block setting (minimumColumnWidth) then columns should resize automatically. But, as I mentioned in PR description, the limitation is that when one of the children has a multi-column span defined, the grid can't resize to a number of columns below that span. Is that what you mean? Or are you testing with the columnCount option like Post Template block has?

@ramonjd
Copy link
Member

ramonjd commented Feb 1, 2024

But, as I mentioned in PR description, the limitation is that when one of the children has a multi-column span defined, the grid can't resize to a number of columns below that span. Is that what you mean? Or are you testing with the columnCount option like Post Template block has?

Oh thanks for explaining. I was comparing things directly with the column stack toggle.

I was fooling around with fairly elaborate grids whose children had individual col/row span values. I suppose I was just musing about whether grid could be reverted on the containing block for narrower viewport widths.

2024-02-02.10.43.08.mp4

Not a biggie, just curious.

@tellthemachines
Copy link
Contributor Author

I suppose I was just musing about whether grid could be reverted on the containing block for narrower viewport widths.

I guess there could be a toggle like in the Columns block, but whether there should be or how it should work is part of the wider conversation about media queries and intrinsic design. In any case I'd leave that for a subsequent iteration 😅

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.

Very cool exploration! It feels really nice being able to set a block to span multiple columns 👍

One known limitation of this approach combined with the auto columns behaviour is that, if we define a span of 3 columns for a grid child and shrink the screen down to a size where less than 3 columns total fit, the grid will keep the 3 columns and overflow the viewport if needed.

In testing this feels like it could be a bit of a blocker to me, as I could imagine folks reaching for column span and not realising that they've broken their layout for smaller viewport sizes. Or were you imagining that we could land this feature, still hidden behind the Grid feature flag, so that we're not exposing it to users until we've got an intrinsic design / responsive logic in place? If so, that could work! It'd give a bit more flexibility if we can continue to make changes behind the feature flag just in case the rules need to change to support future responsive behaviour 🤔

Here's how the smaller viewport size looked for me when using col span of 3:

image

Also, for some reason the column span didn't appear to be output on the site frontend for me. Here's the test markup I was using:

Some block markup with a paragraph spanning 3 cols within a Group block
<!-- wp:group {"layout":{"type":"grid"}} -->
<div class="wp-block-group"><!-- wp:paragraph {"backgroundColor":"accent-3"} -->
<p class="has-accent-3-background-color has-background">A paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"accent-4"} -->
<p class="has-accent-4-background-color has-background">Another paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"contrast-3"} -->
<p class="has-contrast-3-background-color has-background">Another paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"accent-4"} -->
<p class="has-accent-4-background-color has-background">Yet another one</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"style":{"layout":{"columnSpan":"3"}},"backgroundColor":"accent-3"} -->
<p class="has-accent-3-background-color has-background">Another paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"accent-2"} -->
<p class="has-accent-2-background-color has-background">And another one still</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
Editor Site frontend
image image

This is a really exciting feature, though!

@@ -557,46 +557,48 @@ function gutenberg_incremental_id_per_prefix( $prefix = '' ) {
function gutenberg_render_layout_support_flag( $block_content, $block ) {
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );
$block_supports_layout = block_has_support( $block_type, array( 'layout' ), false ) || block_has_support( $block_type, array( '__experimentalLayout' ), false );
$layout_from_parent = $block['attrs']['style']['layout']['selfStretch'] ?? null;
// If there is any value in style -> layout, the block has a child layout.
$layout_from_parent = $block['attrs']['style']['layout'] ?? null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit re: naming: I know the variable wasn't introduced in this PR, but $layout_from_parent makes me think of extracting the layout attribute from the parent. Would it be worth naming this something like $child_layout_style to match $child_layout_declarations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good point! It's a tiny change so I'll just add it here.

@tellthemachines
Copy link
Contributor Author

Thanks for testing @andrewserong! Should work correctly in the front end now.

In testing this feels like it could be a bit of a blocker to me, as I could imagine folks reaching for column span and not realising that they've broken their layout for smaller viewport sizes.

It might currently be more useful for grids using columnCount instead of minimumColumnWidth, as those aren't expected to change the column number on resize.

For the minimumColumnWidth case, and until span clamping is implemented, we can perhaps add a container query based on the min column width and span values, that resets the grid-column rule once the container has a width of less than columnSpan * minimumColumnWidth (we'd have to account for the gap value in there too).

@tellthemachines tellthemachines mentioned this pull request Feb 5, 2024
37 tasks
@fabiankaegy
Copy link
Member

I'll just note here that I've been using a similar concept in a custom block for a while and we quickly found that grid row & column span really needs responsove controls... We were not able to find a good way to manage it without that as grids pretty much always need to reflow on smaller screens to almost a single column and when the row span it set to be larger than 1 on mobile it causes odd behaviors...

Love seeing this exploration though :)

@tellthemachines
Copy link
Contributor Author

Ok folks I added in a container query based on column span and parent grid column width to reset the span value once the grid shrinks to under that number of columns. A couple of disclaimers:

  • Currently only works in the front end; I'll add the logic for the editor once we're sure of the approach;
  • The query width isn't super precise because it doesn't take gap into account, so I'm compensating by adding an extra column into the calculation.

I guess we could use gap but it would introduce a lot of complexity because we'd have to grab the default value from theme.json if the parent block doesn't have a specific value set.

I'm also not super proud of how we're accessing the parent grid's number of columns by adding it as an attribute to the child but I couldn't find another way to get a value from the parent block in the context that render_layout_support_flag runs.

Any and all feedback appreciated!

$container_query_value = $container_query_value . $parent_column_unit;

$child_layout_styles[] = array(
'query' => "@container (max-width: $container_query_value )",
Copy link
Member

@ramonjd ramonjd Feb 6, 2024

Choose a reason for hiding this comment

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

This is similar to what we were testing all those moons ago for CSS layers:

https://github.com/WordPress/gutenberg/pull/41424/files#diff-b85df651c33208862897162a87698bad76f15535f6432b97b022a72e99230edeR112

Very nice to see that you've come up with a similar approach intuitively. 🍺

I think it'd be worth digging deeper to see if we can use a container query or "better_name" to nest other CSS rules, e.g., in CSS layers or nesting itself

I'd be very keen to help!

Copy link
Member

Choose a reason for hiding this comment

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

we were testing all those moons ago for CSS layers

To elaborate a little, much of the work relies on thinking of a way to structure the rules store, e.g., using arrays to represent nested hierarchies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok ok I think I found the generic name for what we want: at-rule ☺️

Regarding the storage structure, we can always change it later if it becomes evident that there's an advantage to representing nested hierarchies with arrays. The API wouldn't change either way; that's an implementation detail. The important thing now is making sure we have the right API.

Copy link
Member

Choose a reason for hiding this comment

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

Ok ok I think I found the generic name for what we want: at-rule ☺️

Yes! Love it.

The important thing now is making sure we have the right API.

💯

@ramonjd ramonjd added the [Package] Style Engine /packages/style-engine label Feb 6, 2024
/**
* If column span is set, it should be removed on small grids.
*/
if ( isset( $block['attrs']['style']['layout']['columnSpan'] ) && $block['attrs']['style']['layout']['parentColumnWidth'] ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need an isset here too?

@tellthemachines
Copy link
Contributor Author

Ok I think that's all the feedback addressed!

The only thing remaining here is to work out what to do about taking blockGap into account for the container query sizing. I'd love to find a viable way of doing that because it would really make the responsiveness smoother.

Current obstacles are:

  • How to fetch the base global value from theme.json (or the core default if that isn't set), because the parent block attributes won't reflect that value.
  • How to deal with values like min(2.5rem, 3vw)? I guess we'd have to use whatever absolute value is available.
  • If blockGap and minimumColumnWidth use different units, how to reconcile them?

Alternatively I can just try to use a sensible default (?) blockGap value for the calculation. The core default of 24px might be adequate.

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.

Speaking in terms of the style engine changes, I really like the direction, thanks for kicking it off @tellthemachines .

I ran a few tests on the output and it handles various rules I threw at it 👍🏻

My comments are mainly brain dribble, but I think it might be good to do a very quick follow up, or maybe extract the style engine changes to another PR with tests in order to isolate the conceptual discussions and longer-term implications.

I have something going on over at:

I don't want to block progress on this PR, and I'm happy to work on an immediate follow up if time is tight, however it would be good to get feedback and ideas from @andrewserong and @aristath.

if ( ! empty( $options['context'] ) ) {
WP_Style_Engine::store_css_rule( $options['context'], $css_rule['selector'], $css_rule['declarations'] );
WP_Style_Engine::store_css_rule( $options['context'], $css_rule['selector'], $css_rule['declarations'], $at_rule );
Copy link
Member

Choose a reason for hiding this comment

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

Disclaimer: No blockers, I'm just thinking of how we could follow up on these changes

  • It would be good to bring this functionality to all style engine public API functions, e.g., wp_style_engine_get_styles should also be able to generate nested rules
  • The same we we allow for combining declarations on already-existing rules, we could think about how to do the same for nested containers, e.g., an "at-rule/container" already exists, any incoming rules with the same container are added to it.

@@ -358,11 +358,11 @@ protected static function is_valid_style_value( $style_value ) {
*
* @return void.
*/
public static function store_css_rule( $store_name, $css_selector, $css_declarations ) {
public static function store_css_rule( $store_name, $css_selector, $css_declarations, $css_at_rule = '' ) {
Copy link
Member

@ramonjd ramonjd Feb 8, 2024

Choose a reason for hiding this comment

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

Also for an after-the-fact-refactor:

If we create a new class of rule, e.g., a rule container with the same interface as WP_Style_Engine_CSS_Rule we might not need to change the function signature of any store methods. In that case we could let the WP_Style_Engine_Processor do the bulk of the work.

I did some experimenting with this last night:

https://github.com/WordPress/gutenberg/pull/58797/files#diff-f23bfb067de6325fe30b9ec68f807342a3286e40b87937bcb806ea285e0252a3R175

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow. What are the advantages, and what would the API look like in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just catching up — is the question say, what might the output look like if there's more than one rule that belongs to a particular at_rule?

So, theoretically, if we were to have an at rule for a particular query, it'd be good to group them all together into the one at_rule instead of outputting multiple?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my comment doesn't really make much sense. 😅 I was only thinking out loud and unfiltered of a way to minimize function signature changes.

what would the API look like in that case?

In terms of what this PR adds to wp_style_engine_get_stylesheet_from_css_rules I would change very little, if nothing. That is, passing an at_rule/container string.

The class's public methods are also part of the API, but on a lower level and the docs discourage folks from using them directly anyway, but you know... 😄

is the question say, what might the output look like if there's more than one rule that belongs to a particular at_rule?

Yeah, that could be one advantage of handling nested-rule-like objects in a separate class - it could keep track of its child rule, e.g., https://github.com/WordPress/gutenberg/pull/58797/files#diff-f23bfb067de6325fe30b9ec68f807342a3286e40b87937bcb806ea285e0252a3R205

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we were to have an at rule for a particular query, it'd be good to group them all together into the one at_rule instead of outputting multiple

I've been thinking about this, and am not convinced there's much advantage to it. All these styles are generated on the server, and sent to the browser as part of the HTML document. Repetitive strings are well handled by gzip, so there shouldn't be much perf impact.

For these queries in particular, given that they depend on two dynamic values, there's likely to be a fair amount of variation so consolidating won't make a huge difference anyway.

On the other hand, if we did want to nest a bunch of rules under the same @rule, we'd want to make sure that it's always loaded at the very end of all the non-nested rules, because @rules (barring the special case of layers) don't affect specificity. So if we don't want to have to increase the specificity of selectors nested inside them, we need to make sure they always come after non-nested rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm not saying we shouldn't do it at all, just that there's no particular urgency as it won't make a difference for the current use case.

For layer rules it would be great to have them grouped together. For media queries, I tend to think it's more readable to have rules that apply to the same element grouped, so the current order feels better. But I guess if we have good reason for batching (layers would definitely be that reason) then we should just do it.

Do you need that feature for this PR? I created a quick test, and I'm not sure it's following that rule

I didn't enforce it in any way, apart from making sure the rules were fed into the style engine in the correct order 😄
I'm not sure the current implementation would make that easy to enforce? In any case, it seems to work as it is so I think this would be fine for a follow-up.

Copy link
Member

Choose a reason for hiding this comment

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

just that there's no particular urgency as it won't make a difference for the current use case.

Oh yes, all my babble has been with a quick follow up in mind.

I'm happy to take it on.

It would help in that case to have one or two unit tests to make sure any refactor doesn't break the expected output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...unless we want it to break expected output? In the case of consolidating rules that is. Or would that be an optional setting?

Copy link
Member

@ramonjd ramonjd Feb 8, 2024

Choose a reason for hiding this comment

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

unless we want it to break expected output? In the case of consolidating rules that is.

Oh, I'll break that 😄

A test that reflects the narrow use case of this PR's layout expectations would be great I think, e.g., building a simple nested rule. Doesn't have to be fancy. Just to have an idea of want you want to see in the output.

I think consolidation by default would work, but I'll wait for the reviews on the follow up 😄

Thanks again!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great discussion, folks! The idea of being able to support things like @keyframes in the future would be very cool for either a core feature or a plugin that allows folks to build custom animations. For now, though, I quite like the inclusion of the additional set_at_rule, get_at_rule, and at_rule params in this PR — it feels like stuff that likely won't box us in if we were to explore nested at rules and the like further down the track. +1 for adding a couple of quick tests to ensure we've captured the intent for now 👍

* Otherwise, create a new entry for the at_rule
*/
if ( ! empty( $at_rule ) ) {
if ( isset( $this->css_rules[ "$at_rule $selector" ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Would be great to have a unit test to make sure.

I think we could steal the ones over in:

if ( empty( $this->rules[ "$at_rule $selector" ] ) ) {
$this->rules[ "$at_rule $selector" ] = new WP_Style_Engine_CSS_Rule( $selector, array(), $at_rule );
}
return $this->rules[ "$at_rule $selector" ];
Copy link
Member

Choose a reason for hiding this comment

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

Another thought for later:

The store can store any object and maybe doesn't need to care about a "$at_rule . $selector" combo.

As mentioned below if we stored something like WP_Style_Engine_CSS_Rules_Container with the same interface as WP_Style_Engine_CSS_Rule, the processor could call $rule->get_css() on all the stored "rules".

I'm just thinking out loud, but it might be easier to manipulate the entire rule set if we can quickly discern between which rules are nested and which are not. The obvious feature is nesting rules inside a single container.


if ( empty( $css_declarations ) ) {
return '';
}

if ( ! empty( $at_rule ) ) {
$selector = "{$at_rule}{{$suffix}{$rule_indent}{$selector}{$spacer}{{$suffix}{$css_declarations}{$suffix}{$rule_indent}}{$suffix}}";
Copy link
Member

Choose a reason for hiding this comment

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

Should be consistent about prettify?

I ran a test to check spacing and indenting and see this:

+@supports (border-style: dotted){
+.gandalf {
+       color: grey;
+       height: 90px;
+       border-style: dotted;
+       align-self: safe center;
+}
+}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I wasn't really paying attention, just copied it from the other line 😅
What does {$suffix} do again?

Copy link
Member

Choose a reason for hiding this comment

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

Good question!

I noticed last night that {$suffix} might be badly named, or is a hangover from another intention and wasn't renamed.

As far as I can tell, it means $new_line or "\n". 🤔

@tellthemachines
Copy link
Contributor Author

Ok folks thanks for the reviews so far! I've addressed all the feedback from the last round so I think this is ready for another round 😄

@SaxonF
Copy link
Contributor

SaxonF commented Feb 12, 2024

Testing well for me.

The dimensions and layout panels need some general refining at some point but what do you think about moving the span controls above minimum height for now?

The only other thing which is not a blocker to shipping is when you increase minimum width to a point where it pushes everything to one column, if you have an item that spans multiple columns, it should probably collapse to one column too as it throws everything off. This is just default grid behaviour, and a bit of an edge case, but something to think about in future.

column-span.mp4

I think we're in a decent spot and can ship once code reviews are good. This will be really useful once we have a manual setting on grid layout.

@tellthemachines
Copy link
Contributor Author

when you increase minimum width to a point where it pushes everything to one column, if you have an item that spans multiple columns, it should probably collapse to one column too as it throws everything off.

I couldn't quite reproduce what shows in your video, could you share the markup? I did add the container query for the editor so multi-column blocks should collapse to one when the grid is single-column, but I'm not sure how well it works when resizing the grid itself, vs resizing the browser window 😅

$container_query_value = $container_query_value . $parent_column_unit;

$child_layout_styles[] = array(
'at_rule' => "@container (max-width: $container_query_value )",
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this'll need a rebase now that #58922 has landed and for this to be updated to rules_group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

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.

This is still testing nicely for me, and thanks for all the follow-up on the rounds of feedback! I think this looks like a good place to land it to me. There are a couple of small things that would be nice to follow-up on before Grid gets taken out of the experiment (seeing if we can infer the parent column width value, possibly tweak the scroll behaviour over the input fields for col / row span), but they don't feel like blockers to landing this initial PR to me.

LGTM! ✨

<InputControl
size={ '__unstable-large' }
label={ __( 'Column Span' ) }
type="number"
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, not a blocker for this PR, but it might be good to swap this out for NumberControl after all to hide the browser default number spinners. Otherwise with one of these fields focused, if you go to scroll the mouse, the number can scroll unexpectedly:

2024-02-14.15.12.18.mp4

Alternately we could potentially add a max value, too? In any case, not to worry about for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I don't think hiding the spinners will remove the scrolling behaviour, at least playing with storybook it seems to persist. I'm not sure we'd want to remove it in any case? It's default input behaviour so folks will be expecting it to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's default input behaviour so folks will be expecting it to work.

Ah, fair enough! Might just be me who found it a bit confusing 🙂

@@ -557,44 +560,95 @@ function gutenberg_incremental_id_per_prefix( $prefix = '' ) {
function gutenberg_render_layout_support_flag( $block_content, $block ) {
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );
$block_supports_layout = block_has_support( $block_type, array( 'layout' ), false ) || block_has_support( $block_type, array( '__experimentalLayout' ), false );
$layout_from_parent = $block['attrs']['style']['layout']['selfStretch'] ?? null;
// If there is any value in style -> layout, the block has a child layout.
$child_layout = $block['attrs']['style']['layout'] ?? null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now whenever I see the nullish coalescing operator, all I think is "one day we'll be able to use this properly"! 😄

(This one seems fine to leave in to me since it was already there)

@tellthemachines tellthemachines merged commit 4bf1d21 into trunk Feb 14, 2024
64 checks passed
@tellthemachines tellthemachines deleted the add/grid-child-spans branch February 14, 2024 04:52
@github-actions github-actions bot added this to the Gutenberg 17.8 milestone Feb 14, 2024
@tellthemachines
Copy link
Contributor Author

Thanks for the reviews folks! Follow-ups to come for both refactoring the parent column logic and adding an "auto/manual" toggle for grids.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Layout Layout block support, its UI controls, and style output. Needs Dev Note Requires a developer note for a major WordPress release cycle [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants