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

Refactor responsive logic for grid column spans. #59057

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Refactor responsive logic for grid column spans. #59057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 25 additions & 3 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,13 +596,16 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {

/**
* If columnSpan is set, and the parent grid is responsive, i.e. if it has a minimumColumnWidth set,
* the columnSpan should be removed on small grids.
* the columnSpan should be removed on small grids. If there's a minimumColumnWidth, the grid is responsive.
* But if the minimumColumnWidth value wasn't changed, it won't be set. In that case, if columnCount doesn't
* exist, we can assume that the grid is responsive.
*/
if ( isset( $block['attrs']['style']['layout']['columnSpan'] ) && isset( $block['attrs']['style']['layout']['parentColumnWidth'] ) ) {
if ( isset( $block['attrs']['style']['layout']['columnSpan'] ) && ( isset( $block['parentLayout']['minimumColumnWidth'] ) || ! isset( $block['parentLayout']['columnCount'] ) ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also check that $block['parentLayout']['type'] is set and is grid? This is potentially edge casey, but if I take a block that is a child of a grid layout and then drag it to be within another group (i.e. within a Row block elsewhere on the page), then the block still has columnSpan set, and the parentLayout does not have columnCount (because the parent layout is flex), but this condition still passes, which results in rules being output that likely shouldn't be:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! So I opted for not doing that because $block['parentLayout']['type'] isn't always set. If grid is defined as the default layout type for a block, and no customisations are made, $block['parentLayout'] will be empty.
I don't think it's a huge issue because the rules we're applying to children (of both flex and grid) are specific to that layout type, so there shouldn't be any weird side-effects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Yeah, these rules don't wind up affecting anything visually, so seems fine to leave it for now 👍

$column_span_number = floatval( $block['attrs']['style']['layout']['columnSpan'] );
$parent_column_width = $block['attrs']['style']['layout']['parentColumnWidth'];
$parent_column_width = isset( $block['parentLayout']['minimumColumnWidth'] ) ? $block['parentLayout']['minimumColumnWidth'] : '12rem';
$parent_column_value = floatval( $parent_column_width );
$parent_column_unit = explode( $parent_column_value, $parent_column_width );

/**
* If there is no unit, the width has somehow been mangled so we reset both unit and value
* to defaults.
Expand Down Expand Up @@ -887,6 +890,25 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
return $processor->get_updated_html();
}

/**
* Add a `render_block_data` filter to fetch the parent block layout data.
*/
add_filter(
'render_block_data',
function ( $parsed_block, $source_block, $parent_block ) {
/**
* Check if the parent block exists and if it has a layout attribute.
* If it does, add the parent layout to the parsed block.
*/
if ( $parent_block && isset( $parent_block->parsed_block['attrs']['layout'] ) ) {
$parsed_block['parentLayout'] = $parent_block->parsed_block['attrs']['layout'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Editing the $parsed_block in a filter is messing with Interactivity API server side rendering.

Variables comparing function:
https://github.com/WordPress/wordpress-develop/blob/9a616a573434b432a5efd4de21039250658371fe/src/wp-includes/interactivity-api/interactivity-api.php#L49

Filter:
https://github.com/WordPress/wordpress-develop/blob/9a616a573434b432a5efd4de21039250658371fe/src/wp-includes/interactivity-api/interactivity-api.php#L69

I'm just writing this so I can follow it up on Monday, I need to work on a way to prevent SSR processing errors when someone filters a parsed_block (Thinking about giving last priority possible, as we compare in render_block filter, after all render_block_data happened. )

Copy link
Member

Choose a reason for hiding this comment

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

For the record, we are comparing the parsed_block array ourselves because modifying it (adding a key like $parsed_block['is_root_interactive_block'] = true) broke the md5 comparison done in the block supports:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up! Yeah the issue with elements block supports already surfaced (see comment above) and was fixed in #59535. render_block_data is a publicly available filter which any extension could use to alter the content of $parsed_block so we can't assume it to be static. The fix will be added to core in 6.6; I guess the Interactivity API logic should also be updated accordingly.

Copy link
Member

@luisherranz luisherranz Mar 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Awesome! So, we will be able to set something like $parsed_block['is_root_interactive_block'] = true during render_block_data, and the check if it exists during render_block, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that should work fine now!

}
return $parsed_block;
},
10,
3
);

// Register the block support. (overrides core one).
WP_Block_Supports::get_instance()->register(
'layout',
Expand Down
16 changes: 4 additions & 12 deletions packages/block-editor/src/components/child-layout-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,10 @@ export default function ChildLayoutControl( {
} ) {
const { selfStretch, flexSize, columnSpan, rowSpan } = childLayout;
const {
type: parentLayoutType,
minimumColumnWidth = '12rem',
columnCount,
} = parentLayout;

/**
* If columnCount is set, the parentColumnwidth isn't needed because
* the grid has a fixed number of columns with responsive widths.
*/
const parentColumnWidth = columnCount ? null : minimumColumnWidth;
type: parentType,
default: { type: defaultParentType = 'default' } = {},
} = parentLayout ?? {};
const parentLayoutType = parentType || defaultParentType;

useEffect( () => {
if ( selfStretch === 'fixed' && ! flexSize ) {
Expand Down Expand Up @@ -122,7 +116,6 @@ export default function ChildLayoutControl( {
onChange( {
rowSpan,
columnSpan: value,
parentColumnWidth,
} );
} }
value={ columnSpan }
Expand All @@ -135,7 +128,6 @@ export default function ChildLayoutControl( {
onChange={ ( value ) => {
onChange( {
columnSpan,
parentColumnWidth,
rowSpan: value,
} );
} }
Expand Down
14 changes: 7 additions & 7 deletions packages/block-editor/src/components/global-styles/dimensions-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ function useHasChildLayout( settings ) {
defaultParentLayoutType === 'grid' ||
parentLayoutType === 'grid' ) &&
allowSizingOnChildren;

return !! settings?.layout && support;
}

Expand Down Expand Up @@ -397,13 +396,16 @@ export default function DimensionsPanel( {
// Child Layout
const showChildLayoutControl = useHasChildLayout( settings );
const childLayout = inheritedValue?.layout;
const { selfStretch } = childLayout ?? {};
const { orientation = 'horizontal' } = settings?.parentLayout ?? {};
const {
type: parentType,
default: { type: defaultParentType = 'default' } = {},
} = settings?.parentLayout ?? {};
const parentLayoutType = parentType || defaultParentType;
const flexResetLabel =
orientation === 'horizontal' ? __( 'Width' ) : __( 'Height' );
const childLayoutResetLabel = selfStretch
? flexResetLabel
: __( 'Grid spans' );
const childLayoutResetLabel =
parentLayoutType === 'flex' ? flexResetLabel : __( 'Grid spans' );
const setChildLayout = ( newChildLayout ) => {
onChange( {
...value,
Expand All @@ -418,7 +420,6 @@ export default function DimensionsPanel( {
flexSize: undefined,
columnSpan: undefined,
rowSpan: undefined,
parentColumnWidth: undefined,
} );
};
const hasChildLayoutValue = () => !! value?.layout;
Expand All @@ -434,7 +435,6 @@ export default function DimensionsPanel( {
flexSize: undefined,
columnSpan: undefined,
rowSpan: undefined,
parentColumnWidth: undefined,
} ),
spacing: {
...previousValue?.spacing,
Expand Down
17 changes: 10 additions & 7 deletions packages/block-editor/src/hooks/layout-child.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import { useSelect } from '@wordpress/data';
*/
import { store as blockEditorStore } from '../store';
import { useStyleOverride } from './utils';
import { useLayout } from '../components/block-list/layout';

function useBlockPropsChildLayoutStyles( { style } ) {
const shouldRenderChildLayoutStyles = useSelect( ( select ) => {
return ! select( blockEditorStore ).getSettings().disableLayoutStyles;
} );
const layout = style?.layout ?? {};
const { selfStretch, flexSize, columnSpan, rowSpan, parentColumnWidth } =
layout;
const { selfStretch, flexSize, columnSpan, rowSpan } = layout;
const parentLayout = useLayout() || {};
const { columnCount, minimumColumnWidth } = parentLayout;
const id = useInstanceId( useBlockPropsChildLayoutStyles );
const selector = `.wp-container-content-${ id }`;

Expand All @@ -37,13 +39,14 @@ function useBlockPropsChildLayoutStyles( { style } ) {
}`;
}
/**
* If parentColumnWidth is set, the grid is responsive
* so a container query is needed for the span to resize.
* If minimumColumnWidth is set on the parent, or if no
* columnCount is set, the grid is responsive so a
* container query is needed for the span to resize.
*/
if ( columnSpan && parentColumnWidth ) {
if ( columnSpan && ( minimumColumnWidth || ! columnCount ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the PHP side of things, do we also need to check that the parent layout type is grid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could actually do this here because the parentLayout comes from the BlockList context and seems to always have the type, but not sure if it's worth having different logic here and in the PHP.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I wouldn't worry about it for now. We can always follow up if need be!

// Calculate the container query value.
const columnSpanNumber = parseInt( columnSpan );
let parentColumnValue = parseFloat( parentColumnWidth );
let parentColumnValue = parseFloat( minimumColumnWidth );
/**
* 12rem is the default minimumColumnWidth value.
* If parentColumnValue is not a number, default to 12.
Expand All @@ -52,7 +55,7 @@ function useBlockPropsChildLayoutStyles( { style } ) {
parentColumnValue = 12;
}

let parentColumnUnit = parentColumnWidth?.replace(
let parentColumnUnit = minimumColumnWidth?.replace(
parentColumnValue,
''
);
Expand Down
Loading