Gallery block: add gap support / block spacing #38164
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { BlockList } from '@wordpress/block-editor'; | ||
| import { useContext, createPortal } from '@wordpress/element'; | ||
|
|
||
| export default function GapStyles( { blockGap, clientId } ) { | ||
| const styleElement = useContext( BlockList.__unstableElementContext ); | ||
|
|
||
| const gap = blockGap | ||
| ? `#block-${ clientId } { --wp--style--unstable-gallery-gap: ${ blockGap } }` | ||
| : `#block-${ clientId } { --wp--style--unstable-gallery-gap: var( --wp--style--block-gap, 0.5em ) }`; | ||
|
|
||
| const GapStyle = () => { | ||
| return <style>{ gap }</style>; | ||
| }; | ||
|
|
||
| return gap && styleElement | ||
| ? createPortal( <GapStyle />, styleElement ) | ||
| : null; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,12 +32,53 @@ function block_core_gallery_data_id_backcompatibility( $parsed_block ) { | |
|
|
||
| add_filter( 'render_block_data', 'block_core_gallery_data_id_backcompatibility' ); | ||
|
|
||
| /** | ||
| * Adds a style tag for the --wp--style--unstable-gallery-gap var. | ||
| * | ||
| * The Gallery block needs to recalculate Image block width based on | ||
| * the current gap setting in order to maintain the number of flex columns | ||
| * so a css var is added to allow this. | ||
| * | ||
| * @param array $attributes Attributes of the block being rendered. | ||
| * @param string $content Content of the block being rendered. | ||
| * @return string The content of the block being rendered. | ||
| */ | ||
| function block_core_gallery_render( $attributes, $content ) { | ||
| $gap = _wp_array_get( $attributes, array( 'style', 'spacing', 'blockGap' ) ); | ||
| // Skip if gap value contains unsupported characters. | ||
| // Regex for CSS value borrowed from `safecss_filter_attr`, and used here | ||
| // because we only want to match against the value, not the CSS attribute. | ||
| $gap = preg_match( '%[\\\(&=}]|/\*%', $gap ) ? null : $gap; | ||
| $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 Note too that this PCRE pattern isn't doing anything that simple string substitution doesn't do already - why did we use a 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 It sound like 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 There was a problem hiding this comment. Something like Doing a dodgy There was a problem hiding this comment.
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 Well, it may be the case that $content = preg_replace( '/class="/', 'class="' . $class . ' ', $content, 1 );
Yes indeed $w = new WP_HTML_Walker( $html );
$w->next_tag( [ 'class_name' => "wp-block-gallery-{$id}" ] );
$w->add_class( $class );
$content = (string) $w; |
||
| 'class="' . $class . ' ', | ||
| $content, | ||
| 1 | ||
| ); | ||
| $gap_value = $gap ? $gap : 'var( --wp--style--block-gap, 0.5em )'; | ||
andrewserong marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| $style = '.' . $class . '{ --wp--style--unstable-gallery-gap: ' . $gap_value . '}'; | ||
ramonjd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Ideally styles should be loaded in the head, but blocks may be parsed | ||
| // 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. |
||
| function () use ( $style ) { | ||
| echo '<style> ' . $style . '</style>'; | ||
| } | ||
| ); | ||
| return $content; | ||
| } | ||
| /** | ||
| * Registers the `core/gallery` block on server. | ||
| */ | ||
| function register_block_core_gallery() { | ||
| register_block_type_from_metadata( | ||
| __DIR__ . '/gallery', | ||
| array( | ||
| 'render_callback' => 'block_core_gallery_render', | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed this. This should rather have been:
See #38889 and #38891
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied this change in #39983.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix @westonruter