Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new grid layout type #4625

Closed
wants to merge 2 commits into from
Closed

Add new grid layout type #4625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff2b304
Add new grid layout type
tellthemachines Jun 16, 2023
ff36b0a
Add since comment for grid layout
tellthemachines Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/wp-includes/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ function wp_register_layout_support( $block_type ) {
*
* @since 5.9.0
* @since 6.1.0 Added `$block_spacing` param, use style engine to enqueue styles.
* @since 6.3.0 Added grid layout type.
* @access private
*
* @param string $selector CSS selector.
Expand Down Expand Up @@ -287,6 +288,44 @@ function wp_get_layout_style( $selector, $layout, $has_block_gap_support = false
);
}
}
} elseif ( 'grid' === $layout_type ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need @since annotation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ( ! empty( $layout['columnCount'] ) ) {
$layout_styles[] = array(
'selector' => $selector,
'declarations' => array( 'grid-template-columns' => 'repeat(' . $layout['columnCount'] . ', minmax(0, 1fr))' ),
);
} else {
$minimum_column_width = ! empty( $layout['minimumColumnWidth'] ) ? $layout['minimumColumnWidth'] : '12rem';

$layout_styles[] = array(
'selector' => $selector,
'declarations' => array( 'grid-template-columns' => 'repeat(auto-fill, minmax(min(' . $minimum_column_width . ', 100%), 1fr))' ),
);
}

if ( $has_block_gap_support && isset( $gap_value ) ) {
$combined_gap_value = '';
$gap_sides = is_array( $gap_value ) ? array( 'top', 'left' ) : array( 'top' );

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|' ) ) {
Copy link
Contributor

@costdev costdev Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@costdev costdev Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$index_to_splice = strrpos( $process_value, '|' ) + 1;
$slug = _wp_to_kebab_case( substr( $process_value, $index_to_splice ) );
$process_value = "var(--wp--preset--spacing--$slug)";
}
$combined_gap_value .= "$process_value ";
}
$gap_value = trim( $combined_gap_value );

if ( null !== $gap_value && ! $should_skip_gap_serialization ) {
Copy link
Contributor

@costdev costdev Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$layout_styles[] = array(
'selector' => $selector,
'declarations' => array( 'gap' => $gap_value ),
);
}
}
}

if ( ! empty( $layout_styles ) ) {
Expand Down
2 changes: 1 addition & 1 deletion src/wp-includes/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1370,7 +1370,7 @@ protected function get_layout_styles( $block_metadata ) {

if (
! empty( $class_name ) &&
! empty( $base_style_rules )
is_array( $base_style_rules )
) {
// Output display mode. This requires special handling as `display` is not exposed in `safe_style_css_filter`.
if (
Expand Down
22 changes: 22 additions & 0 deletions src/wp-includes/theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,28 @@
}
}
]
},
"grid": {
"name": "grid",
"slug": "grid",
"className": "is-layout-grid",
"displayMode": "grid",
"baseStyles": [
{
"selector": " > *",
"rules": {
"margin": "0"
}
}
],
"spacingStyles": [
{
"selector": "",
"rules": {
"gap": null
}
}
]
}
}
},
Expand Down
Loading