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: Add 'Show template' toggle when editing pages #52674

Merged
merged 16 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* WordPress dependencies
*/
import { useEntityBlockEditor } from '@wordpress/core-data';
import { privateApis as blockEditorPrivateApis } from '@wordpress/block-editor';
import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import { store as editSiteStore } from '../../../store';
import { unlock } from '../../../lock-unlock';
import useSiteEditorSettings from '../use-site-editor-settings';
import usePageContentBlocks from './use-page-content-blocks';

const { ExperimentalBlockEditorProvider } = unlock( blockEditorPrivateApis );

const noop = () => {};

/**
* The default block editor provider for the site editor. Typically used when
* the post type is `'wp_template_part'` or `'wp_template'` and allows editing
* of the template and its nested entities.
*
* If the page content focus type is `'hideTemplate'`, the provider will provide
* a set of page content blocks wrapped in a container that, together,
* mimic the look and feel of the post editor and
* allow editing of the page content only.
*
* @param {Object} props
* @param {WPElement} props.children
*/
export default function DefaultBlockEditorProvider( { children } ) {
const settings = useSiteEditorSettings();

const { templateType, isTemplateHidden } = useSelect( ( select ) => {
const { getEditedPostType } = select( editSiteStore );
const { getPageContentFocusType, getCanvasMode } = unlock(
select( editSiteStore )
);
return {
templateType: getEditedPostType(),
isTemplateHidden:
getCanvasMode() === 'edit' &&
getPageContentFocusType() === 'hideTemplate',
canvasMode: unlock( select( editSiteStore ) ).getCanvasMode(),
};
}, [] );

const [ blocks, onInput, onChange ] = useEntityBlockEditor(
'postType',
templateType
);
const pageContentBlock = usePageContentBlocks( blocks, isTemplateHidden );
return (
<ExperimentalBlockEditorProvider
settings={ settings }
value={
isTemplateHidden && pageContentBlock.length
? pageContentBlock
: blocks
}
onInput={ isTemplateHidden ? noop : onInput }
onChange={ isTemplateHidden ? noop : onChange }
useSubRegistry={ false }
>
{ children }
</ExperimentalBlockEditorProvider>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import { store as editSiteStore } from '../../../store';
import DefaultBlockEditorProvider from './default-block-editor-provider';
import NavigationBlockEditorProvider from './navigation-block-editor-provider';

export default function BlockEditorProvider( { children } ) {
const entityType = useSelect(
( select ) => select( editSiteStore ).getEditedPostType(),
[]
);
if ( entityType === 'wp_navigation' ) {
return (
<NavigationBlockEditorProvider>
{ children }
</NavigationBlockEditorProvider>
);
}
return (
<DefaultBlockEditorProvider>{ children }</DefaultBlockEditorProvider>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* External dependencies
*/
import { renderHook } from '@testing-library/react';
/**
* WordPress dependencies
*/
import { createBlock } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import usePageContentBlocks from '../use-page-content-blocks';

jest.mock( '@wordpress/blocks', () => {
return {
__esModule: true,
...jest.requireActual( '@wordpress/blocks' ),
createBlock( name, attributes = {}, innerBlocks = [] ) {
return {
name,
attributes,
innerBlocks,
};
},
};
} );

describe( 'usePageContentBlocks', () => {
const blocksList = [
createBlock( 'core/group', {}, [
createBlock( 'core/post-title' ),
createBlock( 'core/post-featured-image' ),
createBlock( 'core/query', {}, [
createBlock( 'core/post-title' ),
createBlock( 'core/post-featured-image' ),
createBlock( 'core/post-content' ),
] ),
createBlock( 'core/post-content' ),
] ),
createBlock( 'core/query' ),
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
createBlock( 'core/paragraph' ),
createBlock( 'core/post-content' ),
];
it( 'should return empty array if `isPageContentFocused` is `false`', () => {
const { result } = renderHook( () =>
usePageContentBlocks( blocksList, false )
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
);
expect( result.current ).toEqual( [] );
} );
it( 'should return empty array if `blocks` is undefined', () => {
const { result } = renderHook( () =>
usePageContentBlocks( undefined, true )
);
expect( result.current ).toEqual( [] );
} );
it( 'should return empty array if `blocks` is an empty array', () => {
const { result } = renderHook( () => usePageContentBlocks( [], true ) );
expect( result.current ).toEqual( [] );
} );
it( 'should return new block list', () => {
const { result } = renderHook( () =>
usePageContentBlocks( blocksList, true )
);
expect( result.current ).toEqual( [
{
name: 'core/group',
attributes: {
layout: { type: 'constrained' },
style: {
spacing: {
margin: {
top: '4em', // Mimics the post editor.
},
},
},
},
innerBlocks: [
createBlock( 'core/post-title' ),
createBlock( 'core/post-featured-image' ),
createBlock( 'core/post-content' ),
createBlock( 'core/post-content' ),
],
},
] );
} );
} );
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';
import { createBlock } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import { PAGE_CONTENT_BLOCK_TYPES } from '../../../utils/constants';

/**
* Helper method to iterate through all blocks, recursing into allowed inner blocks.
* Returns a flattened object of transformed blocks.
*
* @param {Array} blocks Blocks to flatten.
* @param {Function} transform Transforming function to be applied to each block. If transform returns `undefined`, the block is skipped.
*
* @return {Array} Flattened object.
*/
function flattenBlocks( blocks, transform ) {
const result = [];
for ( let i = 0; i < blocks.length; i++ ) {
// Since the Query Block could contain PAGE_CONTENT_BLOCK_TYPES block types,
// we skip it because we only want to render stand-alone page content blocks in the block list.
if ( [ 'core/query' ].includes( blocks[ i ].name ) ) {
continue;
}
const transformedBlock = transform( blocks[ i ] );
if ( transformedBlock ) {
result.push( transformedBlock );
}
result.push( ...flattenBlocks( blocks[ i ].innerBlocks, transform ) );
}

return result;
}

/**
* Returns a memoized array of blocks that contain only page content blocks,
* surrounded by a group block to mimic the post editor.
*
* @param {Array} blocks Block list.
* @param {boolean} isPageContentFocused Whether the page content has focus. If `true` return page content blocks. Default `false`.
*
* @return {Array} Page content blocks.
*/
export default function usePageContentBlocks(
blocks,
isPageContentFocused = false
) {
return useMemo( () => {
if ( ! isPageContentFocused || ! blocks || ! blocks.length ) {
return [];
}
return [
createBlock(
'core/group',
{
layout: { type: 'constrained' },
style: {
spacing: {
margin: {
top: '4em', // Mimics the post editor.
},
},
},
},
flattenBlocks( blocks, ( block ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it weird that we're trying to guess the blocks from the content of the template rather than just fixing "core/post-title" + "core/post-content" (like the post editor).

Copy link
Member

Choose a reason for hiding this comment

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

Could you help me understand that statement? Is there something more reliable that could be done here?

I imagine it was approached this way to cover the (possibly unlikely) scenario that there are 2 x site titles or whatever in a template.

Plus there is a bit of guessing required since we don't know how deep or where these blocks are located in a template in advance.

Copy link
Contributor

Choose a reason for hiding this comment

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

rather than just fixing "core/post-title" + "core/post-content" (like the post editor).

By grabbing the real blocks we ensure that the order of the content blocks matches the template so that it more closely resembles the site. I suppose the main value at the moment is that it determines whether or not a featured image is shown, and the order of the title, featured image, and content blocks. While reviewing, I thought it added a nice level of polish.

Copy link
Contributor

Choose a reason for hiding this comment

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

By grabbing the real blocks we ensure that the order of the content blocks matches the template so that it more closely resembles the site

Is that the role of this mode though? I mean, it felt like this mode is more of a "post editor" mode to me where we don't really care about the template, we're just filling the content and title.

Copy link
Member

@ramonjd ramonjd Nov 14, 2023

Choose a reason for hiding this comment

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

I mean, it felt like this mode is more of a "post editor" mode to me where we don't really care about the template, we're just filling the content and title.

We could do it this way.

I'd probably want to confer on the UX with some design folks.

The thinking was that, because we're in the site editor, it would be a more intuitive, seamless experience to have the page content reflect the page template.

So for instance, if I edit the Pages template to have the post title appear underneath the post content, (just because I can) then when I edit that page in the site editor, the page corresponds with the new template structure. I can see my changes.

It might be confusing to have the page content seemingly 'revert' in template preview mode just seconds after I've edited the template, whereas the post editor doesn't have any of the extra context of templates etc so the experience is more "static", if that makes sense.

That's the argument for.

Against, it'd be easier to refactor the code to just make it match the post editor behavior, e.g., just show the required inner blocks...

				[
					createBlock( 'core/post-title' ),
					createBlock( 'core/post-content' ),
				]

Another angle is to remove the template preview mode altogether, I'd be for this. I understand there's a "great unification" going on, where the site editor will be jack of all trades, but I don't know how much value hiding the template in this way adds. Maybe to some folks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how much value hiding the template in this way adds. Maybe to some folks.

I'm not sure how much value it has in its current form, I but I like the principle behind the idea of a more focused writing-mode like experience akin to the post editor, that folks can use from within the site editor. However it's exposed, I think it's important that folks can still set a featured image while editing their post content, whether or not that's in the sidebar or on the editor canvas. One of the neat things about having it in the canvas is that it gets exposed in the list view for free, so it's easier to find / interact with.

if ( PAGE_CONTENT_BLOCK_TYPES[ block.name ] ) {
return createBlock( block.name );
}
} )
),
];
Comment on lines +56 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

Another nit: if no page content blocks are found, then we return an empty group block. This means that if a page's template has no content blocks at all, switching off "Template preview" results in an empty page, because the return value is not an empty array:

image

This is getting very edge-casey territory, and you'd only see this if you go to a page that uses a template that has no content blocks and only after switching off Template preview, so not a blocker to landing for me!

Copy link
Member

Choose a reason for hiding this comment

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

Great find! I think we should follow up with a fix for this.

I'll make a note and get a PR up.

🙇🏻

}, [ blocks, isPageContentFocused ] );
}

This file was deleted.

15 changes: 1 addition & 14 deletions packages/edit-site/src/components/block-editor/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { BlockInspector } from '@wordpress/block-editor';
import { privateApis as editPatternsPrivateApis } from '@wordpress/patterns';

Expand All @@ -10,31 +9,19 @@ import { privateApis as editPatternsPrivateApis } from '@wordpress/patterns';
*/
import TemplatePartConverter from '../template-part-converter';
import { SidebarInspectorFill } from '../sidebar-edit-mode';
import { store as editSiteStore } from '../../store';
import SiteEditorCanvas from './site-editor-canvas';
import getBlockEditorProvider from './get-block-editor-provider';
import BlockEditorProvider from './block-editor-provider';

import { unlock } from '../../lock-unlock';
const { PatternsMenuItems } = unlock( editPatternsPrivateApis );
export default function BlockEditor() {
const entityType = useSelect(
( select ) => select( editSiteStore ).getEditedPostType(),
[]
);

// Choose the provider based on the entity type currently
// being edited.
const BlockEditorProvider = getBlockEditorProvider( entityType );

return (
<BlockEditorProvider>
<TemplatePartConverter />
<SidebarInspectorFill>
<BlockInspector />
</SidebarInspectorFill>

<SiteEditorCanvas />

<PatternsMenuItems />
</BlockEditorProvider>
);
Expand Down

This file was deleted.

Loading
Loading