Gallery block: add gap support #37903
Conversation
glendaviesnz
added
the
[Block] Gallery
|
Size Change: +300 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
…based on gap size
…e width based on gap size" This reverts commit a5f39f7.
…dths in css based on gap value
glendaviesnz
changed the title
glendaviesnz
requested review from
ajitbohra,
ellatrix,
mkaz,
mkevins,
spacedmonkey and
TimothyBJacobs
as code owners
lib/block-supports/layout.php
Outdated
| @@ -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;"; | |||
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.mp4When 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. |
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. |
Ah, good point. Thanks for clarifying that. |
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: 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. |
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! |
There was a problem hiding this comment.
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?
Nice catch, thanks, I will take a look at that. |
…d if block is selected
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. |
|
@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 It seems like the main issue with the There are contexts, such as in the gallery, where flex children may need to recalculate dimensions based on the 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. |
|
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. |
|
@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? |
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.
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 |
ah, 🤦, thanks for the extra detail @youknowriad, I was overlooking the likes of a situation where a column block could in theory set a
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.
I have put up a proof of concept of this approach here. It involves adding another If there is some other way of incorporating |
|
Closing in favour of #38164 |
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-gapas 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?
Screenshots