Gallery block: add gap support #34608
Conversation
glendaviesnz
added
[Type] Enhancement
|
Size Change: +3.42 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for jumping in and starting to use the blockGap support @glendaviesnz! This is testing nicely in the editor and on the front end of my test site using the refactored gallery. I think it's going to enable some nice layouts 🙂
I noticed that with the v1 / non-refactored gallery, opting into the blockGap support exposes the blockGap control in the Dimensions panel, however because that CSS isn't updated to use the --wp--style--block-gap variable, the control doesn't appear to do anything. Do you think it'd be worth seeing if we can somehow hide the control on the old gallery, or should we update its CSS to support it, too?
Also, with the --gallery--block--gutter-size variable, is that one that theme authors are targeting? Once the Layout / blockGap support is more widely used, it looks like this PR will use the gap value to override it. That makes sense to me, but I wasn't sure if it'll cause any issues for theme developers? Alternately, if the --gallery--block--gutter-size variable isn't being used elsewhere, we could potentially update the existing vars to use the blockGap variable instead.
| @@ -3,6 +3,10 @@ | |||
|
|
|||
| // Styles for current version of gallery block. | |||
| .wp-block-gallery.has-nested-images { | |||
| --gallery-block--gutter-size: var( | |||
| --wp--style--block-gap, | |||
| $blocks-block__margin | |||
There was a problem hiding this comment.
Should this fallback value be #{$grid-unit-20} for consistency with the other fallback values below? I think $blocks-block__margin is a Sass variable used only in the Button/Buttons blocks?
Also, it looks like the linting is complaining about the formatting:
[3] packages/block-library/src/gallery/style.scss
[3] 6:30 ✖ Expected newline after ":" with a multi-line declaration declaration-colon-newline-after
[3] 6:36 ✖ Unexpected whitespace after "(" function-parentheses-space-inside
[3] 7:25 ✖ Expected single space after "," function-comma-space-after
[3] 9:1 ✖ Unexpected whitespace before ")" function-parentheses-space-inside
There was a problem hiding this comment.
Should this fallback value be #{$grid-unit-20} for consistency with the other fallback values below? I think $blocks-block__margin is a Sass variable used only in the Button/Buttons blocks?
🤦 Doh, thanks, was a bit confused there - have tidied it up so that the correct fallback $grid-unit-20 is used and it is just declared in one place
There was a problem hiding this comment.
Thanks @glendaviesnz! I think this one also needs the interpolation syntax to render the fallback Sass value. In the CSS the Sass variable is being rendered as-is, instead of being swapped out for the real value:
So it'd be #{$grid-unit-20} instead of $grid-unit-20, which should result in the following:
This var is just declared at the gallery level to avoid having to duplicate the fallback setting everywhere |
I have removed the ToolsPanel for v1 of the gallery as it seems like it would be helpful if we don't have to support any new styles in both versions, and a good reason for people to update to the new format. The only way I could see to do this was by setting the display style on the ToolsPanel element - let me know if you can think of another way to do this using the blocks APIs. |
| @@ -94,6 +94,18 @@ function GalleryEdit( props ) { | |||
| blockEditorStore | |||
| ); | |||
|
|
|||
| // Remove the tools panel for v1 Gallery so we only have to support | |||
| // any new dimension settings, etc. for the new gallery format. | |||
| const toolsPanel = document.getElementsByClassName( | |||
There was a problem hiding this comment.
The CSS rule is working nicely to hide the ToolsPanel, and I like it as a pragmatic workaround for not being able to conditionally update the block's supports settings / alter whether or not the panel is displayed.
This might be overkill (and I'm maybe being overly cautious), but I was wondering if it's worth making the check a little more specific to the block inspector? What do you think about something like the following:
const toolsPanel = document.querySelector( '.block-editor-block-inspector .components-tools-panel' );
There was a problem hiding this comment.
Another case that we probably can't really do much about is whether or not the gap control is displayed in global styles for this block 🤔
|
This is testing pretty well for me @glendaviesnz, just added another comment about the fallback values. I've added Joen as a reviewer, too, since I know he's very keen to see more block gap usage across the blocks! 🙂 |
|
Thanks @andrewserong, I have made the additional changes suggested. |
|
Thanks @glendaviesnz! I'm finishing up early today, but I'll give this another test tomorrow 🙂 |
|
The gallery block is perhaps the block that can benefit the very most from gap suppor, so thanks for taking this on. That also begs the next question: is it time to move on to actually using I wouldn't necessarily be opposed to landing this as an interim step, since it does improve the UI. Though it does resurface the question I just posed in the other PR: is "Dimensions" the right panel to put the block gap control in? Or should it be part of Layout or Spacing? |
Good question, I will do some research on this and see if we will cause any issues by implementing as margin change first and then moving to grid, or if we are better to leapfrog straight to grid now that we don't need to worry about IE support. |
|
@jasmussen I have had a look at switching to grid, but the fact that we set any remainder images that don't fit into the set column size to fill the remaining space in the last row, eg.
is difficult to match in grid. To do this in grid it seems like a lot of nth child calculations are needed: .grid > *:nth-child(3n-2):nth-last-of-type(1) {
grid-column: span 3;
}which would have to be replicated for the 2-8 column settings we have. I may be overlooking a css grid setting that can handle this automatically, but most of the solutions I found for this weren't for a dynamic number of items and columns, or stated that you really should use flex not grid when dealing with this sort of layout. I have switched the PR to use gap in the flex layout rather than the messy setting and unsetting of right margins. Keen to hear if you think we can easily replicate this layout for remainder items in grid. |
|
Thanks for exploring switching the margin rules out for |
|
Wasn't able to test the latest branch today, but thanks for exploring! Just to note, you can use It would also be fine to keep using the margins for the near future, and make this just about a UI improvement rather than a full-on refactor. Gap is something to be excited about both in terms of the simpler UI, but also in terms of the CSS property, but it's okay to take it slowly — the big gallery refactor was already a leap in terms of features 👌 |
Thanks @jasmussen, I have kept it as flexbox for now, but switched to using gap instead of margins. This seems to work well and has simplified the css. |
|
The amount of red here is beautiful! It's an important code refactor that can inject some energy into a block that sorely needs it. But it's also a biggish change — not in code complexity, but in potential impact. How can we best get a few different people test across a few themes and browsers, perhaps even plugins? |
Does anyone have any thoughts on additional testing we should do for this change in relation to gallery themes and plugins? |
|
With the big change that the gallery block is already, and code freeze for 5.9 coming up soon, I think we should defer this change until we have an critical fixes for 5.9 sorted, and we can then decide if we want to include this, or defer until the block is live in 5.9. Going to close for now. |
Description
Adds gap support to allow users to alter the space between images.
Fixes: #33582
Testing
theme.jsonenabled theme with"blockGap": true,set underspacingScreenshots