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

Block Editor: Optimize layout style renderer subscription #55762

Merged
merged 6 commits into from
Nov 10, 2023
Merged

Block Editor: Optimize layout style renderer subscription #55762

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
37e1263
Block Editor: Optimize layout style renderer subscription
Mamaduka Nov 1, 2023
3d688d0
Rename components
Mamaduka Nov 2, 2023
8104743
Swap destructuring with optional chaining
Mamaduka Nov 2, 2023
4935332
Use assignment
Mamaduka Nov 2, 2023
ab23635
Address selector feedback
Mamaduka Nov 2, 2023
9b88482
Feedback
Mamaduka Nov 3, 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
227 changes: 129 additions & 98 deletions packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,62 @@ export const withLayoutControls = createHigherOrderComponent(
'withLayoutControls'
);

function BlockWithLayoutStyles( { block: BlockListBlock, props } ) {
const { name, attributes } = props;
const id = useInstanceId( BlockListBlock );
const { layout } = attributes;
const { default: defaultBlockLayout } =
getBlockSupport( name, layoutBlockSupportKey ) || {};
const usedLayout =
layout?.inherit || layout?.contentSize || layout?.wideSize
? { ...layout, type: 'constrained' }
: layout || defaultBlockLayout || {};
const layoutClasses = useLayoutClasses( attributes, name );

// Higher specificity to override defaults from theme.json.
const selector = `.wp-container-${ id }.wp-container-${ id }`;
const [ blockGapSupport ] = useSettings( 'spacing.blockGap' );
const hasBlockGapSupport = blockGapSupport !== null;

// Get CSS string for the current layout type.
// The CSS and `style` element is only output if it is not empty.
const fullLayoutType = getLayoutType( usedLayout?.type || 'default' );
const css = fullLayoutType?.getLayoutStyle?.( {
blockName: name,
selector,
layout: usedLayout,
style: attributes?.style,
hasBlockGapSupport,
} );

// Attach a `wp-container-` id-based class name as well as a layout class name such as `is-layout-flex`.
const layoutClassNames = classnames(
{
[ `wp-container-${ id }` ]: !! css, // Only attach a container class if there is generated CSS to be attached.
},
layoutClasses
);

const { setStyleOverride, deleteStyleOverride } = unlock(
useDispatch( blockEditorStore )
);

useEffect( () => {
if ( ! css ) return;
setStyleOverride( selector, { css } );
return () => {
deleteStyleOverride( selector );
};
}, [ selector, css, setStyleOverride, deleteStyleOverride ] );

return (
<BlockListBlock
{ ...props }
__unstableLayoutClassNames={ layoutClassNames }
/>
);
}

/**
* Override the default block element to add the layout styles.
*
Expand All @@ -354,76 +410,70 @@ export const withLayoutControls = createHigherOrderComponent(
*/
export const withLayoutStyles = createHigherOrderComponent(
( BlockListBlock ) => ( props ) => {
const { name, attributes } = props;
const blockSupportsLayout = hasLayoutBlockSupport( name );
const disableLayoutStyles = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return !! getSettings().disableLayoutStyles;
} );
const shouldRenderLayoutStyles =
blockSupportsLayout && ! disableLayoutStyles;
const id = useInstanceId( BlockListBlock );
const { layout } = attributes;
const { default: defaultBlockLayout } =
getBlockSupport( name, layoutBlockSupportKey ) || {};
const usedLayout =
layout?.inherit || layout?.contentSize || layout?.wideSize
? { ...layout, type: 'constrained' }
: layout || defaultBlockLayout || {};
const layoutClasses = blockSupportsLayout
? useLayoutClasses( attributes, name )
: null;
// Higher specificity to override defaults from theme.json.
const selector = `.wp-container-${ id }.wp-container-${ id }`;
const [ blockGapSupport ] = useSettings( 'spacing.blockGap' );
const hasBlockGapSupport = blockGapSupport !== null;

// Get CSS string for the current layout type.
// The CSS and `style` element is only output if it is not empty.
let css;
if ( shouldRenderLayoutStyles ) {
const fullLayoutType = getLayoutType(
usedLayout?.type || 'default'
);
css = fullLayoutType?.getLayoutStyle?.( {
blockName: name,
selector,
layout: usedLayout,
style: attributes?.style,
hasBlockGapSupport,
} );
}

// Attach a `wp-container-` id-based class name as well as a layout class name such as `is-layout-flex`.
const layoutClassNames = classnames(
{
[ `wp-container-${ id }` ]: shouldRenderLayoutStyles && !! css, // Only attach a container class if there is generated CSS to be attached.
const blockSupportsLayout = hasLayoutBlockSupport( props.name );
const shouldRenderLayoutStyles = useSelect(
( select ) => {
// The callback returns early to avoid block editor subscription.
if ( ! blockSupportsLayout ) {
return false;
}

return ! select( blockEditorStore ).getSettings()
.disableLayoutStyles;
},
layoutClasses
[ blockSupportsLayout ]
);

const { setStyleOverride, deleteStyleOverride } = unlock(
useDispatch( blockEditorStore )
);

useEffect( () => {
if ( ! css ) return;
setStyleOverride( selector, { css } );
return () => {
deleteStyleOverride( selector );
};
}, [ selector, css, setStyleOverride, deleteStyleOverride ] );
if ( ! shouldRenderLayoutStyles ) {
return <BlockListBlock { ...props } />;
}

return (
<BlockListBlock
{ ...props }
__unstableLayoutClassNames={ layoutClassNames }
/>
<BlockWithLayoutStyles block={ BlockListBlock } props={ props } />
);
},
'withLayoutStyles'
);

function BlockWithChildLayoutStyles( { block: BlockListBlock, props } ) {
const layout = props.attributes.style?.layout ?? {};
const { selfStretch, flexSize } = layout;

const id = useInstanceId( BlockListBlock );
const selector = `.wp-container-content-${ id }`;

let css = '';
if ( selfStretch === 'fixed' && flexSize ) {
css = `${ selector } {
flex-basis: ${ flexSize };
box-sizing: border-box;
}`;
} else if ( selfStretch === 'fill' ) {
css = `${ selector } {
flex-grow: 1;
}`;
}

// Attach a `wp-container-content` id-based classname.
const className = classnames( props.className, {
[ `wp-container-content-${ id }` ]: !! css, // Only attach a container class if there is generated CSS to be attached.
} );

const { setStyleOverride, deleteStyleOverride } = unlock(
useDispatch( blockEditorStore )
);

useEffect( () => {
if ( ! css ) return;
setStyleOverride( selector, { css } );
return () => {
deleteStyleOverride( selector );
};
}, [ selector, css, setStyleOverride, deleteStyleOverride ] );

return <BlockListBlock { ...props } className={ className } />;
Copy link
Member

Choose a reason for hiding this comment

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

There's one thing about the child layout code that is buggy, or at least inefficient: if disableLayoutStyles is true, and if the block has a child layout (i.e., there are valid flexSize or selfStretch values), the corresponding CSS will be created and added to style overrides. But the element won't get the wp-container-content-n CSS class, and therefore the generated CSS won't be applied to any element, it's redundant.

The withLayoutStyles HOC is more efficient: it's doesn't even generate the CSS if layout styles are disabled. It would be nice if withChildLayoutStyles behaved the same way.

Cc @tellthemachines to verify that the argument above is correct and that there's no hidden gotcha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that sounds like a bug, yes. The child layout CSS shouldn't be generated at all if disableLayoutStyles is true.

}

/**
* Override the default block element to add the child layout styles.
*
Expand All @@ -433,52 +483,33 @@ export const withLayoutStyles = createHigherOrderComponent(
*/
export const withChildLayoutStyles = createHigherOrderComponent(
( BlockListBlock ) => ( props ) => {
const { attributes } = props;
const { style: { layout = {} } = {} } = attributes;
const layout = props.attributes.style?.layout ?? {};
Mamaduka marked this conversation as resolved.
Show resolved Hide resolved
const { selfStretch, flexSize } = layout;
const hasChildLayout = selfStretch || flexSize;
const disableLayoutStyles = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return !! getSettings().disableLayoutStyles;
} );
const shouldRenderChildLayoutStyles =
hasChildLayout && ! disableLayoutStyles;

const id = useInstanceId( BlockListBlock );
const selector = `.wp-container-content-${ id }`;
const shouldRenderChildLayoutStyles = useSelect(
( select ) => {
// The callback returns early to avoid block editor subscription.
if ( ! hasChildLayout ) {
return false;
}

let css = '';
return ! select( blockEditorStore ).getSettings()
.disableLayoutStyles;
},
[ hasChildLayout ]
);

if ( selfStretch === 'fixed' && flexSize ) {
css += `${ selector } {
flex-basis: ${ flexSize };
box-sizing: border-box;
}`;
} else if ( selfStretch === 'fill' ) {
css += `${ selector } {
flex-grow: 1;
}`;
if ( ! shouldRenderChildLayoutStyles ) {
return <BlockListBlock { ...props } />;
}

// Attach a `wp-container-content` id-based classname.
const className = classnames( props?.className, {
[ `wp-container-content-${ id }` ]:
shouldRenderChildLayoutStyles && !! css, // Only attach a container class if there is generated CSS to be attached.
} );

const { setStyleOverride, deleteStyleOverride } = unlock(
useDispatch( blockEditorStore )
return (
<BlockWithChildLayoutStyles
block={ BlockListBlock }
props={ props }
/>
);

useEffect( () => {
if ( ! css ) return;
setStyleOverride( selector, { css } );
return () => {
deleteStyleOverride( selector );
};
}, [ selector, css, setStyleOverride, deleteStyleOverride ] );

return <BlockListBlock { ...props } className={ className } />;
},
'withChildLayoutStyles'
);
Expand Down
Loading