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.

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

Site Editor: Apply the right post content layout in the "post only" mode #56542

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
148 changes: 109 additions & 39 deletions packages/editor/src/components/provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import useBlockEditorSettings from './use-block-editor-settings';
import { unlock } from '../../lock-unlock';
import DisableNonPageContentBlocks from './disable-non-page-content-blocks';
import { PAGE_CONTENT_BLOCK_TYPES } from './constants';
import { usePostEditorLayout } from './use-post-editor-layout';

const { ExperimentalBlockEditorProvider } = unlock( blockEditorPrivateApis );
const { PatternsMenuItems } = unlock( editPatternsPrivateApis );
Expand Down Expand Up @@ -60,6 +61,41 @@ function useForceFocusModeForNavigation( navigationBlockClientId ) {
] );
}

/**
* Helper method to extract the post content block from a template.
*
* @param {Array} blocks Template blocks.
*
* @return {Object?} Post content block.
*/
function extractPostContentBlockFromTemplateBlocks( blocks ) {
if ( ! blocks ) {
return undefined;
}
for ( let i = 0; i < blocks.length; i++ ) {
Copy link
Member

Choose a reason for hiding this comment

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

You could use for ... of if you're only ever using the value blocks[ i ]?

// Since the Query Block could contain PAGE_CONTENT_BLOCK_TYPES block types,
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to specify here that Query could contain a Post Content block as we're not looking for all PAGE_CONTENT_BLOCK_TYPES in this function.

// we skip it because we only want to render stand-alone page content blocks in the block list.
if ( blocks[ i ].name === 'core/query' ) {
continue;
}
if ( blocks[ i ].name === 'core/post-content' ) {
return blocks[ i ];
}

if ( blocks[ i ].innerBlocks.length ) {
const postContentBlock = extractPostContentBlockFromTemplateBlocks(
blocks[ i ].innerBlocks
);

if ( postContentBlock ) {
return postContentBlock;
}
}
}

return undefined;
}

/**
* Helper method to extract the post content block types from a template.
*
Expand Down Expand Up @@ -111,45 +147,79 @@ function useBlockEditorProps( post, template, mode ) {
useEntityBlockEditor( 'postType', template?.type, {
id: template?.id,
} );
const blocks = useMemo( () => {
if ( post.type === 'wp_navigation' ) {
return [
createBlock( 'core/navigation', {
ref: post.id,
// As the parent editor is locked with `templateLock`, the template locking
// must be explicitly "unset" on the block itself to allow the user to modify
// the block's content.
templateLock: false,
} ),
];
const postContentBlock =
extractPostContentBlockFromTemplateBlocks( templateBlocks );
const postContentLayout = usePostEditorLayout( postContentBlock );
Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking, what about other style-based block attributes that could be set within a template, e.g. typography, min height, background color, and eventually background image support? Is it worth considering grabbing all the other attributes set on the postContentBlock in addition to figuring out the right layout attributes to use?


const maybeNavigationBlock = useMemo( () => {
if ( post.type !== 'wp_navigation' ) {
return undefined;
}
return [
createBlock( 'core/navigation', {
ref: post.id,
// As the parent editor is locked with `templateLock`, the template locking
// must be explicitly "unset" on the block itself to allow the user to modify
// the block's content.
templateLock: false,
} ),
];
}, [ post.type, post.id ] );

if ( mode === 'post-only' ) {
const postContentBlocks =
extractPageContentBlockTypesFromTemplateBlocks(
templateBlocks
);
return [
createBlock(
'core/group',
{
layout: { type: 'constrained' },
style: {
spacing: {
margin: {
top: '4em', // Mimics the post editor.
},
const maybePostOnlyModeBlocks = useMemo( () => {
if ( mode !== 'post-only' ) {
return undefined;
}
const postContentBlocks =
extractPageContentBlockTypesFromTemplateBlocks( templateBlocks );
const innerBlocks = postContentBlocks.length
? postContentBlocks
: [
createBlock( 'core/post-title' ),
createBlock( 'core/post-content' ),
];
const innerBlocksWithLayout = innerBlocks.map( ( block ) => {
if ( block.name === 'core/post-content' ) {
return createBlock( 'core/post-content', {
layout: postContentLayout,
} );
}
return createBlock(
'core/group',
{
layout: postContentLayout,
},
[ block ]
);
Comment on lines +187 to +193
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, nice, so this wraps any other content blocks in a Group block that uses the same layout as the post content block 👍

I suppose one thing we might want to look into separately (not a blocker for this PR) could be to also attempt to grab the styles or wrapper for the post title block in case a template is centering the post title or doing other styling to it? For the moment, I think going with making the layout consistent with the post content layout is a good way to go.

} );

// This group block is only here to leave some space at the top of the canvas.
return [
createBlock(
'core/group',
{
style: {
spacing: {
margin: {
top: '4em',
},
},
},
postContentBlocks.length
? postContentBlocks
: [
createBlock( 'core/post-title' ),
createBlock( 'core/post-content' ),
]
),
];
},
innerBlocksWithLayout
),
];
}, [ mode, templateBlocks, postContentLayout ] );

// It is important that we don't create a new instance of blocks on every change
// We should only create a new instance if the blocks them selves change, not a dependency of them.
const blocks = useMemo( () => {
if ( maybeNavigationBlock ) {
return maybeNavigationBlock;
}

if ( maybePostOnlyModeBlocks ) {
return maybePostOnlyModeBlocks;
}

if ( rootLevelPost === 'template' ) {
Expand All @@ -158,13 +228,13 @@ function useBlockEditorProps( post, template, mode ) {

return postBlocks;
}, [
templateBlocks,
postBlocks,
maybeNavigationBlock,
maybePostOnlyModeBlocks,
rootLevelPost,
post.type,
post.id,
mode,
postBlocks,
templateBlocks,
] );

const disableRootLevelChanges =
( !! template && mode === 'template-locked' ) ||
post.type === 'wp_navigation' ||
Expand Down
29 changes: 29 additions & 0 deletions packages/editor/src/components/provider/use-post-editor-layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';
import { useSettings } from '@wordpress/block-editor';

export function usePostEditorLayout( templatePostContentBlock ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tellthemachines I'm not entirely sure about the logic to compute the right layout in this hook. Maybe you can confirm, I just tried to copy the logic from the VisualEditor component of the edit-post package which I think you implemented (at least most of it)

Copy link
Contributor

Choose a reason for hiding this comment

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

@tellthemachines might be able to confirm, but the logic looks good to me! (Attempts to use constrained if available, while accounting for legacy inherit property, and falls back to default).

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic is correct, but I don't think we need it because, unless I'm quite mistaken 😅, what we're doing here is just grabbing the Post Content layout from the template and applying it to a newly-created Post Content or Group block.

The convoluted layering of fallbacks is necessary in the post editor because we have to manually determine the correct layout classnames to pass to BlockList so they get added at root level, and also add the correct layout styles for them to the page. Plus we're dealing with support for classic themes and all sorts of intermediate scenarios.

But if here we're just using blocks as containers for the post, the block editor already knows how to interpret their attributes, including the legacy ones 🙂 so we could just grab all the Post Content attributes from the template and add them directly instead, which would incidentally preserve any color or typography changes as @andrewserong commented above.

const [ globalStylesLayout ] = useSettings( 'layout' );
const { layout: templateLayoutDefinition } =
templatePostContentBlock?.attributes ?? {};
const layout = useMemo( () => {
if ( ! templateLayoutDefinition ) {
return globalStylesLayout;
}
return templateLayoutDefinition &&
( templateLayoutDefinition?.type === 'constrained' ||
templateLayoutDefinition?.inherit ||
templateLayoutDefinition?.contentSize ||
templateLayoutDefinition?.wideSize )
? {
...globalStylesLayout,
...templateLayoutDefinition,
type: 'constrained',
}
: { ...globalStylesLayout, ...layout, type: 'default' };
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
}, [ templateLayoutDefinition, globalStylesLayout ] );

return layout;
}