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

Gallery block: add gap support #37903

Closed
wants to merge 8 commits into from
Closed

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Jan 12, 2022

Fixes #20705

Description

Adds block gap support to the Gallery block.

This PR makes use of the gap layout setting added in #37360, but required the addition of a new scoped css var --wp--style--scoped-block-gap as the Gallery needs to have access to the current gap setting in css in order to recalculate the width of the images when the gap size changes.

If this css var is not added, because flex layout is used the number of columns reduces as the gap size increases.

I investigated using css grid instead of flex, and this works without the use of this var as the number of columns can be fixed and images automatically resize ... but, there is no way to easily make orphaned images in the last row span empty rows, eg. if 3 columns set, and only 5 images, the images on the last row should expand to fill 50% each instead of 33%. There are some css hacks to make this work, but accounting for all the permutations of last row % splits when you have 8 columns is practically impossible.

Flex is designed to handle this sort of layout, so adding the new css var in order to allow the gap support to work seems like the best option, but I am open to suggestions on alternative approaches.

How has this been tested?

  • Add a Gallery block and add a number of images
  • Test that the block gap can be adjusted and displays as expected in editor and frontend in both block and non-block themes

Screenshots

gallery-gap

@glendaviesnz glendaviesnz added the [Block] Gallery Affects the Gallery Block - used to display groups of images label Jan 12, 2022
@glendaviesnz glendaviesnz self-assigned this Jan 12, 2022
@github-actions
Copy link

github-actions bot commented Jan 12, 2022

Size Change: +300 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-editor/index.min.js 140 kB -98 B (0%)
build/block-editor/style-rtl.css 14.6 kB +1 B (0%)
build/block-editor/style.css 14.6 kB +1 B (0%)
build/block-library/blocks/gallery/style-rtl.css 1.48 kB -120 B (-7%)
build/block-library/blocks/gallery/style.css 1.48 kB -120 B (-7%)
build/block-library/blocks/navigation/style-rtl.css 1.83 kB +9 B (0%)
build/block-library/blocks/navigation/style.css 1.82 kB +9 B (0%)
build/block-library/index.min.js 165 kB +347 B (0%)
build/block-library/style-rtl.css 10.6 kB -188 B (-2%)
build/block-library/style.css 10.6 kB -189 B (-2%)
build/components/index.min.js 214 kB +1 B (0%)
build/components/style-rtl.css 15.5 kB -8 B (0%)
build/components/style.css 15.5 kB -8 B (0%)
build/core-data/index.min.js 13.3 kB +14 B (0%)
build/edit-post/index.min.js 29.6 kB +6 B (0%)
build/edit-site/index.min.js 37.8 kB +538 B (+1%)
build/edit-widgets/index.min.js 16.5 kB +3 B (0%)
build/editor/index.min.js 38.3 kB +81 B (0%)
build/editor/style-rtl.css 3.71 kB +5 B (0%)
build/editor/style.css 3.71 kB +6 B (0%)
build/server-side-render/index.min.js 1.58 kB +10 B (+1%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 960 B
build/admin-manifest/index.min.js 1.1 kB
build/annotations/index.min.js 2.75 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.12 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.28 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 470 B
build/block-library/blocks/button/editor.css 470 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 292 B
build/block-library/blocks/buttons/editor.css 292 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-template/style-rtl.css 127 B
build/block-library/blocks/comment-template/style.css 127 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.22 kB
build/block-library/blocks/cover/style.css 1.22 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 965 B
build/block-library/blocks/gallery/editor.css 967 B
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 810 B
build/block-library/blocks/image/editor.css 809 B
build/block-library/blocks/image/style-rtl.css 507 B
build/block-library/blocks/image/style.css 511 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 493 B
build/block-library/blocks/media-text/style.css 490 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 649 B
build/block-library/blocks/navigation-link/editor.css 650 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.93 kB
build/block-library/blocks/navigation/editor.css 1.94 kB
build/block-library/blocks/navigation/view.min.js 2.82 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 401 B
build/block-library/blocks/page-list/editor.css 402 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/style-rtl.css 446 B
build/block-library/blocks/post-comments-form/style.css 446 B
build/block-library/blocks/post-comments/style-rtl.css 521 B
build/block-library/blocks/post-comments/style.css 521 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 721 B
build/block-library/blocks/post-featured-image/editor.css 721 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 305 B
build/block-library/blocks/post-template/style.css 305 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 80 B
build/block-library/blocks/post-title/style.css 80 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 389 B
build/block-library/blocks/pullquote/style.css 388 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 245 B
build/block-library/blocks/separator/style.css 245 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 744 B
build/block-library/blocks/site-logo/editor.css 744 B
build/block-library/blocks/site-logo/style-rtl.css 181 B
build/block-library/blocks/site-logo/style.css 181 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 177 B
build/block-library/blocks/social-link/editor.css 177 B
build/block-library/blocks/social-links/editor-rtl.css 670 B
build/block-library/blocks/social-links/editor.css 669 B
build/block-library/blocks/social-links/style-rtl.css 1.32 kB
build/block-library/blocks/social-links/style.css 1.32 kB
build/block-library/blocks/spacer/editor-rtl.css 332 B
build/block-library/blocks/spacer/editor.css 332 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 214 B
build/block-library/blocks/tag-cloud/style.css 215 B
build/block-library/blocks/template-part/editor-rtl.css 560 B
build/block-library/blocks/template-part/editor.css 559 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 908 B
build/block-library/common.css 905 B
build/block-library/editor-rtl.css 10 kB
build/block-library/editor.css 10 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/theme-rtl.css 672 B
build/block-library/theme.css 676 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 46.3 kB
build/compose/index.min.js 11.2 kB
build/customize-widgets/index.min.js 11.4 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 631 B
build/data/index.min.js 7.49 kB
build/date/index.min.js 31.9 kB
build/deprecated/index.min.js 485 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.5 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/style-rtl.css 7.16 kB
build/edit-post/style.css 7.16 kB
build/edit-site/style-rtl.css 6.83 kB
build/edit-site/style.css 6.82 kB
build/edit-widgets/style-rtl.css 4.17 kB
build/edit-widgets/style.css 4.17 kB
build/element/index.min.js 3.29 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 6.58 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.63 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.71 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.8 kB
build/keycodes/index.min.js 1.39 kB
build/list-reusable-blocks/index.min.js 1.72 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 925 B
build/nux/index.min.js 2.08 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.84 kB
build/primitives/index.min.js 924 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 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.65 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 kB
build/shortcode/index.min.js 1.49 kB
build/token-list/index.min.js 639 B
build/url/index.min.js 1.9 kB
build/viewport/index.min.js 1.05 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.15 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@glendaviesnz glendaviesnz changed the title [WIP] Gallery block: add gap support Gallery block: add gap support Jan 13, 2022
@glendaviesnz glendaviesnz marked this pull request as ready for review January 13, 2022 22:01
@@ -93,6 +93,7 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
if ( $has_block_gap_support ) {
$gap_style = $gap_value ? $gap_value : 'var( --wp--style--block-gap, 0.5em )';
$style .= "gap: $gap_style;";
$style .= "--wp--style--scoped-block-gap: $gap_style;";
Copy link
Member

Choose a reason for hiding this comment

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

Interesting approach, thanks, @glendaviesnz !

I see the need for block scoping in relation to gap support, and I believe it might help avoid the issues we saw in #36521

The only thing I'd raise as a potential issue is the coupling with the layout abstraction, that is, the requirement to opt-in to __experimentalLayout.

If we look at this is as a feature to be used elsewhere, and not just the Gallery, I'd imagine some blocks might want to take advantage of a scope block gap CSS var, but I can also imagine that not all blocks would want to have the extra styles associated with the layout rules.

Not only that, I'd be interested to know whether it's sustainable to require theme developers to know that two support rules are required for scoped blockGap to work.

I don't know if these are legitimate concerns. The gap rule is a property of grid, flex, and multi-column layouts, which are..., well, "layouts" so there is that 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions - my reason for putting it there was that currently, you can't get gap support at all unless you opt into layout. It actually took a bit of time to get this PR started as I couldn't work out why the gap support wasn't showing when I had opted into spacing 🤔.

I am guessing to move --wp--style--scoped-block-gap out of layout would require also moving gap, which I assume would take a lot of rehashing of some previous decisions about why this is best placed in layout

Layout does have some limitations I had to override in terms of the gallery, but I think these can be sorted in separate PRs.

Copy link
Member

Choose a reason for hiding this comment

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

For the record - I'm having a related intellectual struggle about the role of layout in another PR: Experimenting with adding writing mode to layout #37932

There's an interesting discussion about a "styles engine", which, one day, might help with issues such as these.

I don't want to raise it as a panacea – we have immediate needs that might, for now, require some compromises such as the ones we're introducing in this PR – but I can easily envision that a centralized rendering engine might be the brain butter we need when making such decisions!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi There! There are indeed interesting challenges here and I recommend you read through this PR a bit to understand the decisions about the coupling #37360 and why we want to avoid CSS variables as well.

--
Basically I have several concerns about this code:

  • Conceptually, I think ideally the layout should provide everything and blocks shouldn't need to access the block gap. It seems that's not possible here according to your PR description (I'd like to try to understand this more and see if we can find an alternative).
  • The question that could be asked with the current implementation is why we need the variable for "flex" layout but not other kind of layouts (we have "flow" as well), and the answer is not clear aside the fact that this implemented to solve a specific case (Gallery block uses flex) and adding the CSS variable in all layouts is a no-go (see linked issue/PR)

--
If we're not able to find any solution without involving the CSS variable, I'd recommend a specific hook for the Gallery block to access its attribute (block gap) and generate the required styles without variables and independently of the "layout" block support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @youknowriad, that is useful feedback.

I think I have explored all the options for solving this without a css var, and haven't found a suitable alternative due to the fact that we have a variable number of columns and images, and require the uneven images to equally distribute on the last row, and due to the fact that the key style that needs to update is the width of the innerBlocks nested images, not the parent gallery block styles ... but I will take a look at using a custom hook and looking for a gallery specific solution rather than trying to solve this within the generic layout code.

@ramonjd
Copy link
Member

ramonjd commented Jan 14, 2022

If this css var is not added, because flex layout is used the number of columns reduces as the gap size increases.

Is that a limitation we need to respect?

I am playing around with, admittedly, huge blockGap values and can fool the column count anyway.

Jan-14-2022.12-11-56.mp4

When changing the blockGap value, I can see that adjusting blockGap controls spacing between images. It doesn't seem like a monumental surprise when my column count is affected. My expectations might not be well tuned though 😄

It all makes me wonder whether we should try for inline gap support again (with the flow responsible for its own layout). I tried something out over at #37964, but it's extremely superficial.

@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Jan 14, 2022

If this css var is not added, because flex layout is used the number of columns reduces as the gap size increases.
Is that a limitation we need to respect

If we don't respect it the column number is reduced as soon as the gap is changed at all, eg. if you have 3 columns selected and increase gap by 2 pixels you will drop to two columns, but the Gallery setting will still say 3 columns, which I think would lead to a lot of confused users. In the case of the extreme settings you note, I think it will be obvious to users at those points that they probably need to reduce the gap or the number of columns.

@ramonjd
Copy link
Member

ramonjd commented Jan 14, 2022

if you have 3 columns selected and increase gap by 2 pixels you will drop to two columns, but the Gallery setting will still say 3 columns

Ah, good point. Thanks for clarifying that.

@glendaviesnz
Copy link
Contributor Author

It all makes me wonder whether we should try for inline gap support again

I don't know if this would help, as that is essentially what the layout code is doing, ie. instead of inlining the gap, it is adding the gap value to a custom class attached to the block wrapper:
Screen Shot 2022-01-14 at 2 37 26 PM

and it would still leave the need to access that value in css somehow in order to resize the images as the gap changes?

The only other way around it I could think of was resetting the image sizes in js as the gap value changes, but that seemed like a lot of overhead, and is tricky to translate to the frontend.

@ramonjd
Copy link
Member

ramonjd commented Jan 14, 2022

is essentially what the layout code is doing, ie. instead of inlining the gap, it is adding the gap value to a custom class attached to the block wrapper:

True, I was mainly musing over ways to decouple things from layout, but you're right: some blocks still need to use the value for other purposes, such as margins etc.

Tricky indeed!

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.

Nice work @glendaviesnz! This is testing pretty well for me so far, and the approach reminds me a little of the user-specific variable for blockGap that @richtabor explored in #36680. Since that PR, @youknowriad merged in the main PR to remove CSS variables for block gap in #37360. This PR demonstrates an interesting problem, though, which is that we still need to be able to use the block gap value to calculate things like width (which could also help with the Buttons block widths, too).

I quite like the idea of introducing the block-scoped CSS variable here, and it's working nicely for me in testing so far. I'm curious if Riad or others have feedback on the trade-offs of introducing it? I think it possibly side-steps the issues we had previously with the CSS variable being used in the Layout margin calculations (#36521) 🤔

The only issue I've run into so far is that within the editor, the media placeholder is still rendered as a child of the Gallery container when the block is not selected, so the bottom gap appears to be too large. Should we add a conditional around this line to only render the component if the block is selected?

image

image

@andrewserong andrewserong added the [Type] Enhancement A suggestion for improvement. label Jan 14, 2022
@glendaviesnz
Copy link
Contributor Author

The only issue I've run into so far is that within the editor, the media placeholder is still rendered as a child of the Gallery container when the block is not selected, so the bottom gap appears to be too large.

Nice catch, thanks, I will take a look at that.

@glendaviesnz
Copy link
Contributor Author

The only issue I've run into so far is that within the editor, the media placeholder is still rendered as a child of the Gallery container when the block is not selected, so the bottom gap appears to be too large.

I have pushed an update for this. I also switched the order and put caption above the media upload as it seems to get a bit lost under the media upload. I am not 100% happy with this yet, as still means a lot of strange space added before media upload, but I can't see any way of removing gap for a specific flex child ... but there are plans to move the upload option to the toolbar, so potentially not much point spending too much time sorting this.

@glendaviesnz
Copy link
Contributor Author

@youknowriad on thinking about your feedback, and also reading back through #37360, I wonder if your concerns might be the result of poor var naming on my part?

To me, the issues with --wp--style--block-gap don't lead to a conclusion that CSS vars should be avoided in general if they are in fact the best solution for a specific use case, and appropriately scoped and named - but maybe there are some wider discussions about issues with CSS vars that I have missed.

It seems like the main issue with the --wp--style--block-gap was conflicting uses of this global var, and my initial naming of the new var in the flex layout of --wp--style--scoped-block-gap would obviously be prone to the same misuse and value clashes, even if scoped just to an individual block. Also, as you note, there was no clear indication why this would be set in the flex layout and not other layout types.

There are contexts, such as in the gallery, where flex children may need to recalculate dimensions based on the gap value of the parent container, such as in the case of the Gallery block where the number of columns needs to be maintained as the gap changes. The use of a CSS var is a very efficient way of doing this.

So I have renamed the var so that it is not only clearly scoped just to the current block, but also clearly named to indicate its purpose --wp--style--block-scoped-flex-gap. This change gives a much clearer indication of why this is only tied to the flex layout, and reduces the chances of it being assigned conflicting values within a block.

I will keep looking for alternatives, but the fact that we can't currently use custom properties in inline styles at the block level limits the potential for using a custom hook in the Gallery to achieve something specific to that block, but there may be some elegant way of achieving it that I am overlooking.

@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Jan 17, 2022

I had a play around with one approach of handling this just at the block level without using a CSS var, but creates a much tighter coupling between the Gallery and Image blocks, so I don't think it is a valid way forward.

@glendaviesnz
Copy link
Contributor Author

@youknowriad did you have any thoughts on the revised naming approach mentioned here, as apposed to the option of having to pass context down to flex children innerBlocks to recalculate width, if css vars are to be avoided?

@youknowriad
Copy link
Contributor

It seems like the main issue with the --wp--style--block-gap was conflicting uses of this global var

No, this is not really the issue, the issue is conflicting with the variable applied to the n+1 level which can happen for blocks no matter how we name the variable used when the customization is applied to the block.

So with the current name, it's not clear to me why it's only for "flex" still, what's special about that layout.

having to pass context down to flex children innerBlocks to recalculate width,

In this PR you do the computation in code, isn't it possible to do it with CSS (like you do with the CSS variable)? If the CSS variable is still mandatory somehow, it still seems like you could simply solve this by generating a --wp--style--unstable-gallery-gap just in the case of the Gallery block?

@glendaviesnz
Copy link
Contributor Author

the issue is conflicting with the variable applied to the n+1 level which can happen for blocks no matter how we name the variable used when the customization is applied to the block.

ah, 🤦, thanks for the extra detail @youknowriad, I was overlooking the likes of a situation where a column block could in theory set a --wp--style--block-scoped-flex-gap which would then be inadvertently inherited by a nested Gallery block that hadn't set its own custom gap.

having to pass context down to flex children innerBlocks to recalculate width,
In this PR you do the computation in code, isn't it possible to do it with CSS (like you do with the CSS variable)?

Neither myself, nor anyone else in my team can see an obvious way in CSS to calculate the width of a flex child based on a varying number of columns, and a dynamic gap value, without having the gap value available as a variable, as in here. We may be overlooking something obvious though, so open to ideas.

If the CSS variable is still mandatory somehow, it still seems like you could simply solve this by generating a --wp--style--unstable-gallery-gap just in the case of the Gallery block?

I have put up a proof of concept of this approach here. It involves adding another <style> block to get around the current issue of not being able to add custom CSS properties to inline styles on block elements due to KSES limitations for non-admin users, but the other advantage of this approach is that it allows us to add gap support to the Gallery now without affecting the saved content at all, other than the blockGap attribute, so we can, in theory, remove this additional <style> and pull it into the style engine if/when that is available without the need for any deprecations.

If there is some other way of incorporating --wp--style--unstable-gallery-gap into the Gallery block without an additional <style> that I am overlooking let me know.

@glendaviesnz
Copy link
Contributor Author

Closing in favour of #38164

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a way to control spacing between gallery images
4 participants