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

Always pass layout to inner blocks #47477

Merged
merged 7 commits into from
Jan 31, 2023
Merged

Always pass layout to inner blocks #47477

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6bd0112
Always pass layout to inner blocks
tellthemachines Jan 27, 2023
b5cc279
Fix migration eligibility rule.
tellthemachines Jan 30, 2023
f301327
Use stable fallback object.
tellthemachines Jan 30, 2023
7dde489
Add deprecation fixture.
tellthemachines Jan 30, 2023
ebe258d
Use existing EMPTY_OBJECT pattern
tellthemachines Jan 30, 2023
f1acfb5
Check if block has layout support before adding layout to context.
tellthemachines Jan 31, 2023
e28c0d7
Simplify deprecation fixture markup.
tellthemachines Jan 31, 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
10 changes: 9 additions & 1 deletion packages/block-editor/src/components/block-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,19 @@ import { BlockEditContextProvider, useBlockEditContext } from './context';
export { useBlockEditContext };

export default function BlockEdit( props ) {
const { name, isSelected, clientId, __unstableLayoutClassNames } = props;
const {
name,
isSelected,
clientId,
attributes = {},
__unstableLayoutClassNames,
} = props;
const { layout = null } = attributes;
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be super edge-casey, but I was wondering if we should check whether or not the block has opted-in to the layout support before passing down its layout attribute in the context? Because this will apply to all blocks, if a 3rd party block happens to use an attribute also named layout, then it will be passed down here, too, without a check for the layout block support.

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 idea, let's do that!

const context = {
name,
isSelected,
clientId,
layout,
__unstableLayoutClassNames,
};
return (
Expand Down
32 changes: 22 additions & 10 deletions packages/block-editor/src/components/inner-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { useBlockEditContext } from '../block-edit/context';
import useBlockSync from '../provider/use-block-sync';
import { store as blockEditorStore } from '../../store';
import useBlockDropZone from '../use-block-drop-zone';

import useSetting from '../use-setting';
/**
* InnerBlocks is a component which allows a single block to have multiple blocks
* as children. The UncontrolledInnerBlocks component is used whenever the inner
Expand All @@ -53,7 +53,7 @@ function UncontrolledInnerBlocks( props ) {
renderAppender,
orientation,
placeholder,
__experimentalLayout,
layout,
} = props;

useNestedSettingsUpdate(
Expand All @@ -64,7 +64,7 @@ function UncontrolledInnerBlocks( props ) {
templateLock,
captureToolbars,
orientation,
__experimentalLayout
layout
);

useInnerBlockTemplateSync(
Expand All @@ -82,17 +82,25 @@ function UncontrolledInnerBlocks( props ) {
[ clientId ]
);

const { allowSizingOnChildren = false } =
const defaultLayoutBlockSupport =
getBlockSupport( name, '__experimentalLayout' ) || {};

const layout = useMemo(
const { allowSizingOnChildren = false } = defaultLayoutBlockSupport;

const defaultLayout = useSetting( 'layout' ) || {};

const usedLayout = layout || defaultLayoutBlockSupport;

const memoedLayout = useMemo(
() => ( {
...__experimentalLayout,
// Default layout will know about any content/wide size defined by the theme.
...defaultLayout,
...usedLayout,
...( allowSizingOnChildren && {
allowSizingOnChildren: true,
} ),
} ),
[ __experimentalLayout, allowSizingOnChildren ]
[ defaultLayout, usedLayout, allowSizingOnChildren ]
);

// This component needs to always be synchronous as it's the one changing
Expand All @@ -103,7 +111,7 @@ function UncontrolledInnerBlocks( props ) {
rootClientId={ clientId }
renderAppender={ renderAppender }
__experimentalAppenderTagName={ __experimentalAppenderTagName }
__experimentalLayout={ layout }
__experimentalLayout={ memoedLayout }
wrapperRef={ wrapperRef }
placeholder={ placeholder }
/>
Expand Down Expand Up @@ -152,8 +160,11 @@ const ForwardedInnerBlocks = forwardRef( ( props, ref ) => {
export function useInnerBlocksProps( props = {}, options = {} ) {
const { __unstableDisableLayoutClassNames, __unstableDisableDropZone } =
options;
const { clientId, __unstableLayoutClassNames: layoutClassNames = '' } =
useBlockEditContext();
const {
clientId,
layout = null,
__unstableLayoutClassNames: layoutClassNames = '',
} = useBlockEditContext();
const isSmallScreen = useViewportMatch( 'medium', '<' );
const { __experimentalCaptureToolbars, hasOverlay } = useSelect(
( select ) => {
Expand Down Expand Up @@ -199,6 +210,7 @@ export function useInnerBlocksProps( props = {}, options = {} ) {

const innerBlocksProps = {
__experimentalCaptureToolbars,
layout,
...options,
};
const InnerBlocks =
Expand Down
9 changes: 0 additions & 9 deletions packages/block-editor/src/layouts/flow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,22 +67,13 @@ export default {
info: alignmentInfo[ alignment ],
} ) );
}
const { contentSize, wideSize } = layout;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double-checking — is the idea that this can be removed now because of the layout object being passed down? From testing, the issue of the wide width post template showing up using content size appears to have returned in this PR. I think the flow layout might still need the logic here (or elsewhere), so that the alignwide and alignfull classnames can be injected.

With TwentyTwentyTwo theme applied, it looks like the post template is being rendered as wide width on the site frontend, but content width in the site editor:

Editor (note 650px) Site frontend (note 1000px)
image image

It looks like on the site frontend the post template block's output has both is-layout-flow and alignwide classes as expected, however in the site editor the alignwide class is not present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That code needs to be removed because it's causing align wide and full controls to always show, even if the parent block doesn't have a content width. The problem is that the layout we're extracting content and wide size from there is the full theme layout, not the block one, so it always includes those properties if they're set by the theme.

The specific problem those lines were fixing should now be fixed by the Query block deprecation. I only checked with the test markup from your PR, but will have a look at TT2 now.


const alignments = [
{ name: 'left' },
{ name: 'center' },
{ name: 'right' },
];

if ( contentSize ) {
alignments.unshift( { name: 'full' } );
}

if ( wideSize ) {
alignments.unshift( { name: 'wide', info: alignmentInfo.wide } );
}

alignments.unshift( { name: 'none', info: alignmentInfo.none } );

return alignments;
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/buttons/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const DEFAULT_BLOCK = {
};

function ButtonsEdit( { attributes, className } ) {
const { fontSize, layout = {}, style } = attributes;
const { fontSize, style } = attributes;
const blockProps = useBlockProps( {
className: classnames( className, {
'has-custom-font-size': fontSize || style?.typography?.fontSize,
Expand All @@ -59,7 +59,7 @@ function ButtonsEdit( { attributes, className } ) {
{ className: preferredStyle && `is-style-${ preferredStyle }` },
],
],
__experimentalLayout: layout,

templateInsertUpdatesSelection: true,
} );

Expand Down
14 changes: 1 addition & 13 deletions packages/block-library/src/comments-pagination/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
Warning,
} from '@wordpress/block-editor';
import { useSelect } from '@wordpress/data';
import { getBlockSupport } from '@wordpress/blocks';
import { PanelBody } from '@wordpress/components';

/**
Expand All @@ -29,21 +28,11 @@ const ALLOWED_BLOCKS = [
'core/comments-pagination-next',
];

const getDefaultBlockLayout = ( blockTypeOrName ) => {
const layoutBlockSupportConfig = getBlockSupport(
blockTypeOrName,
'__experimentalLayout'
);
return layoutBlockSupportConfig?.default;
};

export default function QueryPaginationEdit( {
attributes: { paginationArrow, layout },
attributes: { paginationArrow },
setAttributes,
clientId,
name,
} ) {
const usedLayout = layout || getDefaultBlockLayout( name );
const hasNextPreviousBlocks = useSelect( ( select ) => {
const { getBlocks } = select( blockEditorStore );
const innerBlocks = getBlocks( clientId );
Expand All @@ -64,7 +53,6 @@ export default function QueryPaginationEdit( {
const innerBlocksProps = useInnerBlocksProps( blockProps, {
template: TEMPLATE,
allowedBlocks: ALLOWED_BLOCKS,
__experimentalLayout: usedLayout,
} );

// Get the Discussion settings
Expand Down
2 changes: 0 additions & 2 deletions packages/block-library/src/gallery/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ const linkOptions = [
];
const ALLOWED_MEDIA_TYPES = [ 'image' ];
const allowedBlocks = [ 'core/image' ];
const LAYOUT = { type: 'default', alignments: [] };

const PLACEHOLDER_TEXT = Platform.isNative
? __( 'ADD MEDIA' )
Expand Down Expand Up @@ -531,7 +530,6 @@ function GalleryEdit( props ) {
allowedBlocks,
orientation: 'horizontal',
renderAppender: false,
__experimentalLayout: LAYOUT,
...nativeInnerBlockProps,
} );

Expand Down
1 change: 0 additions & 1 deletion packages/block-library/src/group/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ function GroupEdit( {
renderAppender: hasInnerBlocks
? undefined
: InnerBlocks.ButtonBlockAppender,
__experimentalLayout: layoutSupportEnabled ? usedLayout : undefined,
__unstableDisableLayoutClassNames: ! layoutSupportEnabled,
}
);
Expand Down
6 changes: 0 additions & 6 deletions packages/block-library/src/navigation/edit/inner-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ const DEFAULT_BLOCK = {
name: 'core/navigation-link',
};

const LAYOUT = {
type: 'default',
alignments: [],
};

export default function NavigationInnerBlocks( {
clientId,
hasCustomPlaceholder,
Expand Down Expand Up @@ -131,7 +126,6 @@ export default function NavigationInnerBlocks( {
parentOrChildHasSelection
? InnerBlocks.ButtonBlockAppender
: false,
__experimentalLayout: LAYOUT,
placeholder: showPlaceholder ? placeholder : undefined,
}
);
Expand Down
15 changes: 2 additions & 13 deletions packages/block-library/src/post-content/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,11 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';
import {
useBlockProps,
useInnerBlocksProps,
useSetting,
__experimentalRecursionProvider as RecursionProvider,
__experimentalUseHasRecursion as useHasRecursion,
store as blockEditorStore,
Warning,
} from '@wordpress/block-editor';
import { useEntityProp, useEntityBlockEditor } from '@wordpress/core-data';
Expand Down Expand Up @@ -39,16 +36,9 @@ function ReadOnlyContent( { userCanEdit, postType, postId } ) {
);
}

function EditableContent( { layout, context = {} } ) {
function EditableContent( { context = {} } ) {
const { postType, postId } = context;
const themeSupportsLayout = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return getSettings()?.supportsLayout;
}, [] );
const defaultLayout = useSetting( 'layout' ) || {};
const usedLayout = ! layout?.type
? { ...defaultLayout, ...layout, type: 'default' }
: { ...defaultLayout, ...layout };

const [ blocks, onInput, onChange ] = useEntityBlockEditor(
'postType',
postType,
Expand All @@ -61,7 +51,6 @@ function EditableContent( { layout, context = {} } ) {
value: blocks,
onInput,
onChange,
__experimentalLayout: themeSupportsLayout ? usedLayout : undefined,
}
);
return <div { ...props } />;
Expand Down
14 changes: 1 addition & 13 deletions packages/block-library/src/query-pagination/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
store as blockEditorStore,
} from '@wordpress/block-editor';
import { useSelect } from '@wordpress/data';
import { getBlockSupport } from '@wordpress/blocks';
import { PanelBody } from '@wordpress/components';

/**
Expand All @@ -28,21 +27,11 @@ const ALLOWED_BLOCKS = [
'core/query-pagination-next',
];

const getDefaultBlockLayout = ( blockTypeOrName ) => {
const layoutBlockSupportConfig = getBlockSupport(
blockTypeOrName,
'__experimentalLayout'
);
return layoutBlockSupportConfig?.default;
};

export default function QueryPaginationEdit( {
attributes: { paginationArrow, layout },
attributes: { paginationArrow },
setAttributes,
clientId,
name,
} ) {
const usedLayout = layout || getDefaultBlockLayout( name );
const hasNextPreviousBlocks = useSelect( ( select ) => {
const { getBlocks } = select( blockEditorStore );
const innerBlocks = getBlocks( clientId );
Expand All @@ -61,7 +50,6 @@ export default function QueryPaginationEdit( {
const innerBlocksProps = useInnerBlocksProps( blockProps, {
template: TEMPLATE,
allowedBlocks: ALLOWED_BLOCKS,
__experimentalLayout: usedLayout,
} );
return (
<>
Expand Down
76 changes: 75 additions & 1 deletion packages/block-library/src/query/deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,80 @@ const v3 = {
},
};

const deprecated = [ v3, v2, v1 ];
const v4 = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a fixture to match this added deprecation? (e.g. core__query__deprecated-4.html etc)
It might be a little tricky if the deprecation depends on an earlier version of useInnerBlocksProps.save() to make sure that we test the deprecation is being run correctly 🤔

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, I always forget about the fixtures 😅

attributes: {
queryId: {
type: 'number',
},
query: {
type: 'object',
default: {
perPage: null,
pages: 0,
offset: 0,
postType: 'post',
order: 'desc',
orderBy: 'date',
author: '',
search: '',
exclude: [],
sticky: '',
inherit: true,
taxQuery: null,
parents: [],
},
},
tagName: {
type: 'string',
default: 'div',
},
displayLayout: {
type: 'object',
default: {
type: 'list',
},
},
namespace: {
type: 'string',
},
},
supports: {
align: [ 'wide', 'full' ],
html: false,
color: {
gradients: true,
link: true,
__experimentalDefaultControls: {
background: true,
text: true,
},
},
__experimentalLayout: true,
},
save( { attributes: { tagName: Tag = 'div' } } ) {
const blockProps = useBlockProps.save();
const innerBlocksProps = useInnerBlocksProps.save( blockProps );
return <Tag { ...innerBlocksProps } />;
},
isEligible: ( { layout } ) =>
! layout || layout.inherit || layout.contentSize,
Copy link
Contributor

Choose a reason for hiding this comment

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

By including layout.contentSize in the eligibility list, what happens if a user sets a content size after it's migrated to a constrained layout? Will this match happen another time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question! I re-used the same logic from the Group block deprecation which seems to be working fine, but will double-check.

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, turns out it does run so I added an extra check to only run it if content size is set without constrained layout.

migrate: ( attributes ) => {
const { layout = null } = attributes;
if ( ! layout ) {
return attributes;
}
if ( layout.inherit || layout.contentSize ) {
return {
...attributes,
layout: {
...layout,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that if layout.inherit is true, then this still gets included in the object here. Should we add a inherit: undefined line here after ...layout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's any harm in keeping it. We've been keeping it for the Group block deprecation, and on the whole I think the less we change the less scope there is for things to go wrong 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

The main reason I thought of removing it is that a truthy check for layout.inherit is included in the isEligible callbacks, so I was wondering if it'd be possible for a migrated block to be treated as though it hasn't been migrated if the inherit value sticks around 🤔

Totally agree with trying to change fewer things, though!

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 can also change the check to not run at all if type: "contstrained" is present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why other blocks don't have a deprecation except for Query? It's weird to have an empty Query Loop with all the attributes set.. Also the migrate function should run only to this deprecation? Usually it's needed to also run for other migrations..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Query deprecation doesn't address this change, it should have been added as part of #42763. I added it here in an attempt to fix the issue reported in #43502, as the solution from that PR was causing other breakage.

Also the migrate function should run only to this deprecation? Usually it's needed to also run for other migrations...

I'm not sure I understand. Is there anything I should change here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tested, but this is from the docs:

It is important to note that if a deprecation’s save method does not produce a valid block then it is skipped completely, including its migrate method, even if isEligible would return true for the given attributes. This means that if you have several deprecations for a block and want to perform a new migration, like moving content to InnerBlocks, you may need to update the migrate methods in multiple deprecations in order for the required changes to be applied to all previous versions of the block.

You could test by copy/paste the other deprecated versions and see if there are errors.

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, got it, thanks! I'll have a look.

type: 'constrained',
},
};
}
},
};

const deprecated = [ v4, v3, v2, v1 ];

export default deprecated;
Loading