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.

Sign up for GitHub

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

Gallery block: add gap support #37903

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a691007
Add gap support to gallery block
Jan 12, 2022
a5f39f7
Switch to using css grid so we don't need to recalculate image width …
Jan 12, 2022
aa2c19d
Revert "Switch to using css grid so we don't need to recalculate imag…
Jan 13, 2022
588f6a1
Add custom gap property to layout styles
Jan 13, 2022
3cbb53f
Add a scoped block gap css var to allow gallery to calculate image wi…
Jan 13, 2022
b6afcaf
Override the flex align-items setting
Jan 13, 2022
8adf044
Switch order of caption and media upload, and only render media uploa…
Jan 14, 2022
48a8b86
Improve naming of flex gap css var
Jan 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference-guides/core-blocks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ Display multiple images in a rich gallery. ([Source](https://github.com/WordPres

- **Name:** core/gallery
- **Category:** media
- **Supports:** align, anchor
- **Supports:** align, anchor, spacing (blockGap), units (em, px, rem, vh, vw)
- **Attributes:** allowResize, caption, columns, fixedHeight, ids, imageCrop, images, linkTarget, linkTo, shortCodeTransforms, sizeSlug

## Group
Expand Down
1 change: 1 addition & 0 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

} else {
$style .= 'gap: 0.5em;';
}
Expand Down
3 changes: 3 additions & 0 deletions packages/block-editor/src/layouts/flex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ export default {
${ appendSelectors( selector ) } {
display: flex;
gap: ${ hasBlockGapStylesSupport ? blockGapValue : '0.5em' };
--wp--style--scoped-block-gap: ${
hasBlockGapStylesSupport ? blockGapValue : '0.5em'
};
flex-wrap: ${ flexWrap };
${ orientation === 'horizontal' ? rowOrientation : columnOrientation }
}
Expand Down
17 changes: 16 additions & 1 deletion packages/block-library/src/gallery/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,22 @@
},
"supports": {
"anchor": true,
"align": true
"align": true,
"units": [ "px", "em", "rem", "vh", "vw" ],
"spacing": {
"blockGap": true,
"__experimentalDefaultControls": {
"blockGap": true
}
},
"__experimentalLayout": {
"allowSwitching": false,
"allowInheriting": false,
"allowEditing": false,
"default": {
"type": "flex"
}
}
},
"editorStyle": "wp-block-gallery-editor",
"style": "wp-block-gallery"
Expand Down
50 changes: 12 additions & 38 deletions packages/block-library/src/gallery/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,24 @@
// Import styles for rendering the static content of deprecated gallery versions.
@import "./deprecated.scss";

// The following is a temporary override until flex layout supports
// an align items setting of normal.
figure.wp-block-gallery.has-nested-images {
align-items: normal;
}
// Styles for current version of gallery block.
.wp-block-gallery.has-nested-images {
display: flex;
flex-wrap: wrap;
// Need bogus :not(#individual-image) to override long :not()
// specificity chain on default image block on front end.
figure.wp-block-image:not(#individual-image) {
// Add space between thumbnails, and unset right most thumbnails later.
margin: 0 var(--gallery-block--gutter-size, #{$grid-unit-20}) var(--gallery-block--gutter-size, #{$grid-unit-20}) 0;

&:last-of-type:not(#individual-image) {
margin-right: 0;
}

width: calc(50% - (var(--gallery-block--gutter-size, #{$grid-unit-20}) / 2));

&:nth-of-type(even) {
margin-right: 0;
}
width: calc(50% - (var(--wp--style--scoped-block-gap, #{$grid-unit-20}) / 2));
}

figure.wp-block-image {
display: flex;
flex-grow: 1;
justify-content: center;
position: relative;
margin-top: auto;
margin-bottom: auto;
flex-direction: column;

> div,
Expand Down Expand Up @@ -98,11 +88,11 @@
margin-top: 0;
margin-bottom: auto;
img {
margin-bottom: var(--gallery-block--gutter-size, #{$grid-unit-20});
margin-bottom: var(--wp--style--scoped-block-gap, #{$grid-unit-20});
}

figcaption {
bottom: var(--gallery-block--gutter-size, #{$grid-unit-20});
bottom: var(--wp--style--scoped-block-gap, #{$grid-unit-20});
}
}
}
Expand All @@ -125,43 +115,27 @@
}

&.columns-1 figure.wp-block-image:not(#individual-image) {
margin-right: 0;
width: 100%;
}

// Beyond mobile viewports, we allow up to 8 columns.
@include break-small {
@for $i from 3 through 8 {
&.columns-#{ $i } figure.wp-block-image:not(#individual-image) {
margin-right: var(--gallery-block--gutter-size, #{$grid-unit-20});
width: calc(#{math.div(100%, $i)} - (var(--gallery-block--gutter-size, #{$grid-unit-20}) * #{math.div($i - 1, $i)}));
width: calc(#{math.div(100%, $i)} - (var(--wp--style--scoped-block-gap, #{$grid-unit-20}) * #{math.div($i - 1, $i)}));

}

// Prevent collapsing margin while sibling is being dragged.
&.columns-#{$i} figure.wp-block-image:not(#individual-image).is-dragging ~ figure.wp-block-image:not(#individual-image) {
margin-right: var(--gallery-block--gutter-size, #{$grid-unit-20});
}
}
// Unset the right margin on every rightmost gallery item to ensure center balance.
@for $column-count from 1 through 8 {
&.columns-#{$column-count} figure.wp-block-image:not(#individual-image):nth-of-type(#{ $column-count }n) {
margin-right: 0;
}
}
// If number of columns not explicitly set default to 3 columns if 3 or more images.
&.columns-default {
figure.wp-block-image:not(#individual-image) {
margin-right: var(--gallery-block--gutter-size, #{$grid-unit-20});
width: calc(33.33% - (var(--gallery-block--gutter-size, 16px) * #{math.div(2, 3)}));
}
figure.wp-block-image:not(#individual-image):nth-of-type(3n+3) {
margin-right: 0;

width: calc(33.33% - (var(--wp--style--scoped-block-gap, 16px) * #{math.div(2, 3)}));
}
// If only 2 child images use 2 columns.
figure.wp-block-image:not(#individual-image):first-child:nth-last-child(2),
figure.wp-block-image:not(#individual-image):first-child:nth-last-child(2) ~ figure.wp-block-image:not(#individual-image) {
width: calc(50% - (var(--gallery-block--gutter-size, 16px) * 0.5));
width: calc(50% - (var(--wp--style--scoped-block-gap, 16px) * 0.5));
}
// For a single image set to 100%.
figure.wp-block-image:not(#individual-image):first-child:nth-last-child(1) {
Expand Down