Try replacing flex with grid in Gallery block. #60022
Conversation
tellthemachines
added
[Block] Gallery
tellthemachines
requested review from
noisysocks,
SaxonF,
richtabor,
MaggieCabrera,
ramonjd and
andrewserong
|
Size Change: -223 B (-0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I just tested in the editor, adding new galleries.
It's impressive, and the controls work as I'd expect straight away both in manual and auto types. 🎉
2024-03-21.13.49.26.mp4
One thing I noticed was, when adding columns using the inserter or deleting columns, the columns revert to 2 - is this just reverting to the current column default value or is there sensible grid calculation to maintain the user's column selection?
Also, I think this is a trunk thing, but I've been trying to increase the row span of a grid item that occupies the max number of columns. Nothing happens in the visualizer but the row span value increases. I'm not sure if that's normal or not.
2024-03-21.14.30.40.mp4
Overall, I like the direction. It's a big improvement to have the ability to arrange gallery images using the drag handlers.
| // Update the image attributes without creating new undo levels. | ||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| updateBlockAttributes( image.clientId, { | ||
| style: { |
There was a problem hiding this comment.
I know it's a bit off topic from the stated intention of the PR, but making a note as it affects the testing experience.
I'm not sure if it's here, but some image styles are being stripped
So if I add styles to each of the images, e.g., borders those attributes being filtered out after saving. Also affects galleries created in trunk.
| Trunk | This PR |
|---|---|
There was a problem hiding this comment.
Oh yes, probably there, thanks for spotting it!
There was a problem hiding this comment.
Very cool exploration! The calculation between desired column count and target columns is pretty clever, too. Is the plan to implement that in the server-rendering, too? If so, what would the relationship be between the image block's logic and the layout support's? I.e. just wondering if the calculation logic "belongs" to the image block or the layout support, or a bit of both 🤔
Play with the grid controls and give constructive feedback on the experience 😅
The main point of friction I ran into is the disconnect between the target columns selected and the calculated columns. If we go with the internal-to-the-layout-support idea of calculating the number of grid columns, how do we go about presenting it in the inspector sidebar? At the moment sliding the Settings columns input up and down and seeing it go to 21 columns feels unexpected, even if we need that value to get it to work internally.
2024-03-25.16.02.54.mp4
Also, it then lays out the larger number of columns in the editor canvas, so dragging to a desired column span doesn't lock to the size of the user's selected number of columns. In the case of the Gallery block, I think I'd expect the grid as it appears on the editor canvas to match the number of columns selected, rather than the larger number of partial or in-between grid lines. I.e. if I have a 7 column gallery block, I'd expect to see a grid of 7 columns, not the calculated 21 on the editor canvas. Would it be worth trying to show the "fake" number of columns to the user in the canvas interactivity, or is better to go with the real grid number?
I also noticed that the grid doesn't collapse down the way the flex one does in narrower viewports, as it currently extends off the side of the viewport. I wasn't sure if that's a known issue since this still a WIP.
In terms of potential breakage for themes out in the wild, I did a quick search for themes that specify .wp-block-gallery. It looks like most adjust things like margin or padding, so wouldn't interfere with switching from flex to grid. However, it looks like there are at least a few such as Blogus that manually apply flex related things. In that theme's case, it outputs the following to force to a 2 column layout:
/* Block Style */
.wp-block-gallery{
display: flex;
flex-wrap: wrap;
margin: 0;
justify-content: space-between;
}
.wp-block-gallery .wp-block-image{
max-width: calc(50% - 5px);
flex-basis: calc(50% - 5px);
margin: 0;
}
In testing locally, the layout block support's display: grid overrides that theme's styles, so the forcing to 2 columns doesn't wind up being applied. So, if we go with switching to Grid, rather than adding Grid as an option in addition to Flex, it'll likely be a breaking change for some themes out in the wild.
What do you think the trade-offs are between interpolating the flex-based column count to the grid-based one as in this PR, versus treating Grid as an additional layout type that folks can switch to? The magic conversion between target column count and the grid one seems very clever to me, but I'm curious about the complexity involved to replicate what flex does out of the box. As an example, what would the drawbacks be of having a layout type toggle, or what if we kept things "flex" until a user interacts with the grid controls in some way? Just an idea!
|
Thanks for the feedback and testing, folks!
@ramonjd do you mean adding blocks? Or adding columns via the Gallery block control? Either way I couldn't repro it, would be great to have some specific steps or a screengrab of this! I have noticed wonky behaviour when adding/deleting blocks, but haven't been able to repro it consistently either 😅
The only thing that needs some server-side tweaking is placing the Image block span attributes correctly when the Gallery has random order toggled on (random order gets set on the server, so we need to replace whatever attributes the Image had before with the correct ones for their new placement). Aside from that, everything just works! How I plan to tackle the random order problem is by adding the array of children and their respective spans to block context, so that it's available on the server side without having to duplicate the complex calculations. So we shouldn't have to touch Image blocks at all. But it's also an interesting idea to try and move the calculations to the layout support itself, because we could leverage the behaviour for other blocks. So that's definitely an avenue to explore!
This is one of the questions I'm hoping design folks can give their input on! Perhaps we extend the layout support in a way that allows for "columns" to not correspond to the actual number of columns in the grid and use the existing layout panel control (so in that case you'd have no way of manipulating the actual column number manually) but whether that should be a toggle or something else (maybe not a UI control at all but just something determined by the block support config) remains to be decided.
Yes I think ideally we'd visualise the fake columns.
Hmm, there's no responsive behaviour yet. The current one should be easy enough to emulate, but perhaps we can improve on it. I don't see galleries extending past the viewport, though my local starts horizontally scrolling below 360px regardless of the presence of galleries 😅
I'd have to test this, but I think the specificity reduction in #60228 will fix it, given the theme styles are loaded after global/layout styles. Otherwise, I guess there's the same potential for breakage as with any CSS change, so it'll have to be clearly flagged. It might also be good to look into what these themes are trying to do and see if the grid layout solves the problem for them, or if there's anything we can add to it that might do so!
If we can replicate current behaviour as the baseline, I don't think there's any advantage to preserving the flex layout. I don't think there'd be any advantage to exposing flex vs grid as a choice to users either (they shouldn't care about the implementation). The main disadvantage of keeping both side by side is code complexity, and related increasing of chances of stuff breaking. |
|
Big +1 to Andrew's comments about the interplay between the two 'columns' settings. That was my main point of friction too. I tend to agree that the target columns setting should always be respected, regardless of grid layout columns. It feels very unexpected when the canvas does not match the target columns. I appreciate that can get tricky in certain situations though:
Could the first point be addressed by saying that the min value of the grid columns setting = the current target? Similar for the second point; maybe grid columns can only be multiples of the target? Would that be too restrictive? I suppose that could get strange if you set the target to 1, but in that case you probably wouldn't need the grid settings anyway and they could be hidden? For the third point, the only thing I can think to do is add some help text to the control which explains the setting is a target, and if you adjust the widths of individual images then the target won't always be attainable. 
It could be conditional similar to the color contrast warning. IE only appear when a child has a custom width, and include button to reset the custom widths: 
To be honest I don't love any of these suggestions, and there could be issues I'm not seeing with them all – there are many moving parts here. It may need a step back and deeper design exploration. Some minor observations:
resize.mp4 |
|
A few notes:
gallery02.mp4
|
Sorry, yes, blocks 🤦🏻 - specifically, adding image blocks via the inbetween inserter, and also deleting individual image blocks. I've noticed it after adjusting the columns manually. It must be the same thing you've noticed then. 2024-03-2809.38.21-ezgif.com-resize-video.mp4 |
|
Thanks for all the feedback and ideas folks!
These are great points @jameskoster! So what this PR does is calculate the real grid column number based on both the target columns and the number of images in the gallery. This is still a very early stage experiment, but ideally I think that when the target column control is available, the real column control should be hidden. There shouldn't be a need to edit both. Point 3. shouldn't be an issue at all, because the width/height/aspect ratio of Image blocks in a Gallery block isn't editable, and there are styles applied to them to make sure that if an image with existing size attributes is dragged in, those styles won't apply inside the Gallery. Currently this means that users can't really adjust the relative sizes of their images in the Gallery. My hope for this PR is to give them a sensible way to do so with the grid span controls 😄 Currently the resize handles on the images are the ones from the grid experiment (if you have it enabled, otherwise no handles show). I think on-canvas interaction is a big part of making this possible; it would be too clunky to edit column spans manually via the sidebar layout controls.
Absolutely! They will happen, the on-canvas interaction is still in very early stages too.
@richtabor that's a bug, thanks for spotting it! I haven't really done much work on back compat for existing galleries yet, but the intended behaviour is for it to turn into a grid, with however many columns the original gallery has, set in manual mode.
As an engineer, I'm probably not the best person to answer this 😂 but main difference is that in Manual the column number is fixed regardless of screen size, whereas in Auto the columns fill the available space. We could maybe come up with better names for these controls?
No, I think it would be better to hide the columns control in the Layout panel, because the Settings panel controls the visual number of columns, which is a better experience. But ideally, we can make this smart column calculation a part of layout itself, in which case we'd add the visual column control to the layout panel (instead of its current one) and deprecate this one. Does that make sense? I'm thinking of a "smartColumnDistribution" (or whatever better name 😅 ) setting for layout that would enable the items in the grid to be nicely distributed as an initial state, just like happens in Gallery now. |
tellthemachines
force-pushed
the
try/galleries-with-grid
branch
from
6c85786
to
edc72e3
Compare
|
I am in the process of recreating an artist web site here in Norway. Here is an example of a way that she wants one of her gallery pages. |
|
I am hoping that this PR can continue even though Isabel has now gone on a 3 month sabbatical. Thank you. |
|
Thanks for the enthusiasm here, Paal! This was an early proof of concept to get a sense for how it might work, however it's lower down the list of priorities for the Grid block as there are many other issues that need to be addressed for WordPress 6.7. I'd expect this feature not to make it in for 6.7, but would be a good one for later releases. For the current list of tasks for the Grid block / layout block support see this tracking issue: #57478 |
|
Thank you for the feedback Andrew! |
What?
Addresses #42240.
The initial step towards making Gallery layouts easily editable is converting the existing layout logic to use the layout support grid implementation. This requires some calculations to create a grid with enough columns to support a different number of children on each row, and adding columnSpan attributes to the children accordingly.
This is a WIP - it's mostly reproducing the current Gallery layouts for now; some things still need to be addressed:
is-layout-flexclassname)The main goal of this PR is to test the behaviour of the grid layout UI with complex multi-column and multi-span layouts. Ideally, we'd work out reasonably painless ways to do things like:
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast