Gallery block: add gap support / block spacing #38164
Conversation
glendaviesnz
added
[Status] In Progress
|
Size Change: -174 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
| // after that, so loading in the footer for now. | ||
| // See https://core.trac.wordpress.org/ticket/53494. | ||
| add_action( | ||
| 'wp_footer', |
There was a problem hiding this comment.
There are issues with adding this to the footer, but this is the same temporary approach as taken by the layout support here, and can hopefully be sorted as part of the new style engine implementation.
There was a problem hiding this comment.
It's becoming more a more clear that we should look at having a function to do this for us (inject block support style tag) and reuse in layout, duotone and here. (the wp_footer becomes just an implementation detail of it)
There was a problem hiding this comment.
Note here that a function has been introduced on a separate branch to use the right hook (header or footer, depending on the theme), I guess it should be used here.
| @@ -534,6 +536,7 @@ function GalleryEdit( props ) { | |||
| /> | |||
| </BlockControls> | |||
| { noticeUI } | |||
| <style>{ style }</style> | |||
There was a problem hiding this comment.
The additional style tag added inline can cause issues (block gap with previous block ...), ideally it should use the same mechanism used for layout styles to inject the style tags (I think it's a Head slot or something like that)
There was a problem hiding this comment.
Thanks - have switched it out for a similar approach to the layout styles as suggested.
There was a problem hiding this comment.
@youknowriad I have moved this to a portal as used by the layout lib. Do you have any other feedback on how the block gap is now being used in this PR?
glendaviesnz
changed the title
glendaviesnz
removed
the
[Status] In Progress
glendaviesnz
requested review from
ajitbohra,
mkaz and
mkevins
as code owners
glendaviesnz
force-pushed
the
try/gallery-gap-css-var
branch
from
22d0984
to
5422714
Compare
|
This is a general question rather than an observation on this PR, as I think it's an existing issue. Should a block style definition in "styles": {
"blocks": {
"core/gallery": {
"spacing": {
"blockGap": "20px"
}
}
}
}I noticed it while testing blockGap axial support |
|
This is testing well for me @glendaviesnz Thanks for making those changes. 🙇 I don't really understand what's going on with the mobile E2Es. Looking at the Gutenberg commit history I don't see the corresponding test failing elsewhere. The gallery appears in the fixture, and the test is complaining that |
The code in that fixture is the v1 format and for some reason the use of |
glendaviesnz
force-pushed
the
try/gallery-gap-css-var
branch
from
6f0c5ee
to
d8cfac4
Compare
There was a problem hiding this comment.
I used TwentyTwentyTwo and added a Gallery block with ten images.
✅ Altered block gap setting to 1px and it adjusted in the editor correctly.
✅ Published post and looked at it in the front end and the 1px gap appeared correctly.
✅ Modified to a 20px gap in the editor and it appeared correctly in the frontend and editor.
I switched to TwentyTen an old non-block theme and the gap setting was removed. The gap switched to the theme setting and appeared the same in the editor and front end.
glendaviesnz
force-pushed
the
try/gallery-gap-css-var
branch
from
547a46c
to
8ab1544
Compare
There was a problem hiding this comment.
Nice work @glendaviesnz! The gap itself is working pretty well for me, but I noticed a couple of issues.
In the editor, the gap between the images and the figcaption for the gallery appears to be slightly too big. I think this is due to an empty media placeholder wrapper? In the other PR I believe we wrapped the media placeholder in an isSelected check, but I wasn't sure if that approach is still valid with the move to using the Add button in the block toolbar?
The other issue is that I think the e2e test failure for the gallery tests might be legitimate when it comes to the figcaption. I had trouble running the e2e tests locally, but manually stepping through the steps, I noticed that after uploading an image to the gallery with this PR applied, the figcaption moves slightly (where it doesn't on trunk), and if you then go to view the code view of the block and switch back to the visual view, the block throws an error:
Kapture.2022-01-28.at.12.42.42.mp4
I wasn't quite sure what the culprit would be since the diff in this PR doesn't appear to change all that much from the JS side of things, so perhaps there's either a conflict in this block with using the layout support, or it could be to do with the portal? Let me know if you need any other testing info!
Good point @andrewserong - have applied the same fix as in the other PR to not display the empty media placeholder wrapper. The e2e test issue seemed to be caused by the GapStyle not returning null in the instances when it had no content - they are running locally with a fix for this in place, so 🤞 will be ok on CI now also. |
|
Thanks @glendaviesnz, that's fixed the caption gap issue, and I can no longer reproduce the block error when switching between the code and visual editor views 👍 |
|
This is looking GTM! ✅ Gap is applied consistently in the post editor and frontend when set, and falls back to theme.json value |
There was a problem hiding this comment.
This is still working well for me, and a good use case for context-aware CSS vars. Because it's block-specific, I can't imagine any scenario where it would impact a potential refactoring of layout or blockGap, but I'll leave that open to folks who have more insight in that area.
|
Thank you so much for doing this! @getdave can we try to do the same for the navigation block? |
|
Noting that this change breaks back-compat for themes that currently target |
|
I'm also getting this warning when adjusting the block spacing on a clean copy of 12.9: Warning: preg_match() expects parameter 2 to be string, array given in ...wp-content/plugins/gutenberg/build/block-library/blocks/gallery.php on line 51 |
😄 woops - thanks for pointing that out @justintadlock - I think Since the Gallery refactor was released a lot happened in terms of a general block flex gap support, so it made sense to use this for the gallery instead of using the gutter var to adjust margins. The move from I thought we might be able to map any existing It is still possible to override the gap in themes with something like: figure.wp-block-gallery.has-nested-images {
gap: 0px;
}With Looking at https://wpdirectory.net/, in terms of themes and plugins listed on .org there appear to be 2 themes and 2 plugins making use of it, but I will have a closer look at potential impact, and mitigations, as part of writing up the dev note for this. |
|
@glendaviesnz - Thanks for the thorough explanation.
That works for a full override but not for overriding the default. Just with a quick test, it looks like targeting Themes should be able to set a default, but user choice should take precedence. I'll do some more testing to see if I can find the sweet spot in the middle of all that.
I consider anything added to the public code as part of a WordPress release to be important. FWIW, these types of breaking changes are often the most-cited issues I hear when talking to theme authors about why they are not building block themes at all. I understand the reasoning behind this and others, but it puts themers in a tough spot. Ideally, themers could just set the default gallery gap via
I was going to say one of those themes was mine, but it's not appearing in the search at all. So, there's at least a third. :) |
|
@justintadlock thanks for the extra feedback, I will take another good look at this next week to see if any modifications are possible to make it easier for themes to set a default ahead of 6.0. |
| $id = uniqid(); | ||
| $class = 'wp-block-gallery-' . $id; |
There was a problem hiding this comment.
I've applied this change in #39983.
There was a problem hiding this comment.
Thanks for this fix @westonruter
|
Reviewing |
|
A small note that it'd be helpful to name the issue similarly to what displays in the UI. Often folks close to developing this work use the issue name but when folks go looking in the UI, it's not there. Instead, it's block spacing. I understand that logically, but new to WordPress or new to FSE folks may get lost if we commonly refer to one thing and then the same terminology isn't in the UI. |
glendaviesnz
changed the title
Thanks @courtneyr-dev, good point, have renamed for future reference. |
|
Yes definitely, this can be an issue. Add my support to this. Thanks for highlighting it on this issue Courtney.
…________________________________
From: Courtney Robertson ***@***.***>
Sent: 04 May 2022 17:22
To: WordPress/gutenberg ***@***.***>
Cc: Abhanonstopnewsuk ***@***.***>; Comment ***@***.***>
Subject: Re: [WordPress/gutenberg] Gallery block: add gap support (PR #38164)
A small note that it'd be helpful to name the issue similarly to what displays in the UI. Often folks close to developing this work use the issue name but when folks go looking in the UI, it's not there. Instead, it's block spacing. I understand that logically, but new to WordPress or new to FSE folks may get lost if we commonly refer to one thing and then the same terminology isn't in the UI.
—
Reply to this email directly, view it on GitHub<#38164 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AH6IIEB637PJP72JLVJNI3DVIKW6DANCNFSM5MT7SQTA>.
You are receiving this because you commented.Message ID: ***@***.***>
|
|
We could also adjust wording in the UI for consistency as well - whichever way. Great move though so far. |
| $id = uniqid(); | ||
| $class = 'wp-block-gallery-' . $id; | ||
| $content = preg_replace( | ||
| '/' . preg_quote( 'class="', '/' ) . '/', |
There was a problem hiding this comment.
Curious why we have this preg_quote here as it doesn't do anything. We generally never need to RegExp-quote a static string literal, especially if the string value doesn't have the delimiter we're escaping in it, as this one does.
Note too that this PCRE pattern isn't doing anything that simple string substitution doesn't do already - why did we use a preg_replace() here instead of str_replace()?
There was a problem hiding this comment.
I'm not too sure why it was used originally but I believe the implementation here was duplicated from layout.php as it matches the logic used here: https://github.com/WordPress/gutenberg/blob/trunk/lib/block-supports/layout.php#L318, which doesn't appear to have been updated from when the Layout block support was initially introduced back in: #29335, unless I'm missing something.
It sound like str_replace() would work too and be simpler to read. If we update it here it'd be good to update it in the Layout support too for consistency. And of course I'm hopeful for us to be able to switch over to WP_HTML_Walker at some point 😀
There was a problem hiding this comment.
Yes, it was definitely a case of copy pasta from layout, so @youknowriad is potentially the one that knows the reason for the use of preg_replace if there was one.
There was a problem hiding this comment.
Something like $content = str_replace( 'wp-block-gallery', "wp-block-gallery $class", $content ); seems to work in this instance. There are several nested <figure class="..., hence the use of the classname.
Doing a dodgy microtime speed test with 10 galleries on a page, the difference appears negligible on my machine.
There was a problem hiding this comment.
Doing a dodgy microtime speed test with 10 galleries on a page, the difference appears negligible on my machine.
My question was definitely geared towards "this looks obviously wrong so I'm confident that I'm misunderstanding the intent of the code and its implementation" more than "this could potentially be slower than another micro-optimized approach."
Thanks for the clarification y'all. In the meantime, would someone be willing to simplify so this doesn't confuse even more folks? To be clear, I'm not commenting on the method of HTML modification, just on the superfluous use of preg_ functions which emulate str_ varieties.
Well, it may be the case that str_replace() won't only replace a single instance. If that's important we might need to stick with preg_ but we absolutely don't need preg_quote() because it's being passed a literal with no quotable characters.
$content = preg_replace( '/class="/', 'class="' . $class . ' ', $content, 1 );And of course I'm hopeful for us to be able to switch over to #42485 at some point 😀
Yes indeed
$w = new WP_HTML_Walker( $html );
$w->next_tag( [ 'class_name' => "wp-block-gallery-{$id}" ] );
$w->add_class( $class );
$content = (string) $w;
Fixes #20705 & #33582
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--unstable-gallery-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