Add new grid layout type #4625

tellthemachines · 2023-06-16T07:29:54Z

Trac ticket: https://core.trac.wordpress.org/ticket/58554

Introduces the PHP changes from WordPress/gutenberg#49018 (except for the kses filter which will be dealt with separately) and the changes to layout.php from WordPress/gutenberg#49050, because they build upon the changes from the first PR.

To test this change, check out this branch locally and update the following npm packages:

@wordpress/block-editor": "11.8.0"
"@wordpress/block-library": "8.8.0"
"@wordpress/blocks": "12.8.0"

Then run npm run postsync-gutenberg-packages followed by npm run build:dev (assuming you're building from src, otherwise run npm run build)

Then, in the post editor, paste the following block markup:

<!-- wp:group {"layout":{"type":"grid","minimumColumnWidth":"8rem"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>grid</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>row</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>stack</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

The grid should look fine in the editor; on the front end it won't show the correct column number without the kses patch to support repeat.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

andrewserong

This is testing nicely for me with the changes applied along with repeat allowed via #4637:

✅ Grid layout test markup works as expected, using auto-fill rule in grid-template-columns.
✅ Updating the markup to use a columnCount attribute with an integer correctly outputs a grid-template-columns rule with a number based approach for the number of columns.
✅ blockGap output works correctly, and parses spacing presets correctly.

Values are passed to style engine for output, which is responsible for filtering CSS values via safecss_filter_attr, so this all looks good to me!

costdev

Thanks for the PR @tellthemachines! Looks good to me.

Just a couple of questions about some of the checks that exist in these changes and elsewhere in the file. Not a blocker for this PR, but possibly a follow-up improvement in this file depending on the answers?

costdev · 2023-06-20T09:39:51Z

src/wp-includes/block-supports/layout.php

+			}
+			$gap_value = trim( $combined_gap_value );
+
+			if ( null !== $gap_value && ! $should_skip_gap_serialization ) {


I don't think $gap_value can ever be null by this point. Should this be '' !== $gap_value, or is the condition not needed?

Although I see this is how it's done elsewhere in the file. If it can't be null by this point, then this could maybe be updated elsewhere in the file in a separate issue to reduce checks and make things a bit cleaner.

Agreed, better look into cleaning all the checks up at once in a separate issue.

costdev · 2023-06-20T09:44:37Z

src/wp-includes/block-supports/layout.php

+			foreach ( $gap_sides as $gap_side ) {
+				$process_value = is_string( $gap_value ) ? $gap_value : _wp_array_get( $gap_value, array( $gap_side ), $fallback_gap_value );
+				// Get spacing CSS variable from preset value if provided.
+				if ( is_string( $process_value ) && str_contains( $process_value, 'var:preset|spacing|' ) ) {


Won't $process_value always be a string after the line above? If not, what other type might it be, and is this safe when interpolated on the last line of the foreach()?

Looks like $gap_value could be an array, e.g.,

array( 'top' => '12px', // column 'left' => '11px', // row )

So I think the extra string check makes sense if we're fetching the value of $gap_value['top'] for example?

Checking if $gap_value is a string makes sense. I was referring to the is_string( $process_value ). This seems like it's always a string already - The fallback value is documented as a string, values always appear to have units like px or em (such as '12px' if getting top from your example), and $process_value is appended to a string without any conversion at the bottom of the foreach():

$combined_gap_value .= "$process_value "

$process_value might possibly be null, but I don't think it could be anything other than string or null.

This logic is repeated for the flex layout type, so might also be worth looking into separately and fixing all instances together.

Oh sorry, I didn't make it down to $combined_gap_value .= "$process_value ";... now I get it 🤦

I'm pretty sure the assumption is "yes", it's a string, so the extra check is probably redundant (?)

I guess folks could add anything as a style value in theme.json (and they probably do), even if it's contrary to the theme.json schema rules.

ramonjd

LGTM

ramonjd · 2023-06-21T02:55:15Z

src/wp-includes/block-supports/layout.php

 				);
 			}
 		}
+	} elseif ( 'grid' === $layout_type ) {


Need @since annotation

Oh, thanks! I forget about them half the time 😅

ramonjd · 2023-06-21T03:01:41Z

src/wp-includes/block-supports/layout.php

+			foreach ( $gap_sides as $gap_side ) {
+				$process_value = is_string( $gap_value ) ? $gap_value : _wp_array_get( $gap_value, array( $gap_side ), $fallback_gap_value );
+				// Get spacing CSS variable from preset value if provided.
+				if ( is_string( $process_value ) && str_contains( $process_value, 'var:preset|spacing|' ) ) {


Looks like $gap_value could be an array, e.g.,

array( 'top' => '12px', // column 'left' => '11px', // row )

So I think the extra string check makes sense if we're fetching the value of $gap_value['top'] for example?

tellthemachines · 2023-06-22T04:00:38Z

Finally got e2e to pass on this branch so will try committing again 🤞

tellthemachines · 2023-06-22T04:06:55Z

Thanks for the reviews folks!

Committed in r55975 / 6188537.

tellthemachines requested review from azaozz, dream-encode, getsource and hellofromtonya June 19, 2023 05:02

This was referenced Jun 19, 2023

Add kses support for repeat #4637

Closed

Plugin: Synchronize PHP changes for WordPress 6.3 release WordPress/gutenberg#51077

Closed

ramonjd mentioned this pull request Jun 20, 2023

Add fallback layout styles for Post Template #4639

Closed

andrewserong approved these changes Jun 20, 2023

View reviewed changes

costdev approved these changes Jun 20, 2023

View reviewed changes

ramonjd approved these changes Jun 21, 2023

View reviewed changes

tellthemachines force-pushed the backport/grid-layout branch from 30692a2 to 3cf0439 Compare June 21, 2023 04:07

tellthemachines added 2 commits June 22, 2023 13:37

Add new grid layout type

ff2b304

Add since comment for grid layout

ff36b0a

tellthemachines force-pushed the backport/grid-layout branch from 3cf0439 to ff36b0a Compare June 22, 2023 03:38

tellthemachines closed this Jun 22, 2023

Add new grid layout type #4625

Add new grid layout type #4625

Uh oh!

Conversation

tellthemachines commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewserong left a comment

Choose a reason for hiding this comment

Uh oh!

costdev left a comment

Choose a reason for hiding this comment

Uh oh!

costdev Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tellthemachines Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

costdev Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramonjd Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

costdev Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tellthemachines Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

ramonjd Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

ramonjd left a comment

Choose a reason for hiding this comment

Uh oh!

ramonjd Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

tellthemachines Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

ramonjd Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

tellthemachines commented Jun 22, 2023

Uh oh!

tellthemachines commented Jun 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tellthemachines commented Jun 16, 2023 •

edited

Loading

costdev Jun 20, 2023 •

edited

Loading

costdev Jun 20, 2023 •

edited

Loading

costdev Jun 21, 2023 •

edited

Loading