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

Edit Site: Add template loading. #19081

Merged
merged 6 commits into from
Jan 16, 2020
Merged

Edit Site: Add template loading. #19081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77434ae
Edit Site: Add root template ID to editor settings.
epiqueras Dec 11, 2019
7380c69
Edit Site: Load root template in block editor.
epiqueras Dec 11, 2019
2180025
Lib: Update demo block template.
epiqueras Dec 12, 2019
0938cab
Lib: Rename demo block template.
epiqueras Dec 12, 2019
82fe02f
Edit Site: Serialize blocks on persistent changes.
epiqueras Dec 12, 2019
7f8488c
Edit Site: Add multiple entity saving functionality.
epiqueras Dec 13, 2019
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
4 changes: 2 additions & 2 deletions lib/demo-block-templates/index.html → lib/demo-block-templates/front-page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@

<!-- wp:column {"verticalAlignment":"center"} -->
<div class="wp-block-column is-vertically-aligned-center">
<!-- wp:navigation-menu {"customTextColor":"#353fff"} -->
<!-- wp:navigation {"customTextColor":"#353fff"} -->
<!-- wp:navigation-link {"label":"One"} /-->
<!-- wp:navigation-link {"label":"Two"} -->
<!-- wp:navigation-link {"label":"Sub 1"} /-->
<!-- wp:navigation-link {"label":"Sub 2"} /-->
<!-- /wp:navigation-link -->
<!-- wp:navigation-link {"label":"Three"} /-->
<!-- /wp:navigation-menu -->
<!-- /wp:navigation -->
</div>
<!-- /wp:column -->
</div>
Expand Down
7 changes: 7 additions & 0 deletions lib/edit-site-page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class="edit-site"
* @param string $hook Page.
*/
function gutenberg_edit_site_init( $hook ) {
global $_wp_current_template_id;
if ( 'gutenberg_page_gutenberg-edit-site' !== $hook ) {
return;
}
Expand Down Expand Up @@ -73,6 +74,12 @@ function gutenberg_edit_site_init( $hook ) {
$settings['fontSizes'] = $font_sizes;
}

// Get root template by trigerring `./template-loader.php`'s logic.
epiqueras marked this conversation as resolved.
Show resolved Hide resolved
get_front_page_template();
get_index_template();
apply_filters( 'template_include', null );
Copy link
Contributor

@youknowriad youknowriad Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What is this line about? Should we comment?
I would have expected something like

$template_id = get_front_page_template();

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 need to run the filters so that the template-loader code runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this prevent us from having a dedicated function that returns the template?

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 way we hijack the template hierarchy is by filtering the template getter functions to store the template hierarchy in a global and then filtering template_include to return the template canvas with the found block template. There is no straightforward way to "return" the IDs from there.

This code is doing the same thing the front-end renderer does to set all those things in motion, but instead of echoing the template canvas at the end, it just uses the set globals to get the IDs it needs.

$settings['templateId'] = $_wp_current_template_id;

// Initialize editor.
wp_add_inline_script(
'wp-edit-site',
Expand Down
18 changes: 11 additions & 7 deletions lib/template-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ function create_auto_draft_for_template_part_block( $block ) {
* @return string Path to the canvas file to include.
*/
function gutenberg_find_template( $template_file ) {
global $_wp_current_template_content, $_wp_current_template_hierarchy;
global $_wp_current_template_id, $_wp_current_template_content, $_wp_current_template_hierarchy;

// Bail if no relevant template hierarchy was determined, or if the template file
// was overridden another way.
Expand Down Expand Up @@ -149,15 +149,17 @@ function gutenberg_find_template( $template_file ) {
$higher_priority_block_template_priority = PHP_INT_MAX;
$block_template_files = glob( get_stylesheet_directory() . '/block-templates/*.html' ) ?: array();
if ( is_child_theme() ) {
$block_template_files = array_merge( $block_template_files, glob( get_template_directory() . '/block-templates/*.html' ) ?: array() );
$block_template_files = array_merge( $block_template_files, glob( get_template_directory() . '/block-templates/*.html' ) ?: array() );
}
if ( gutenberg_is_experiment_enabled( 'gutenberg-full-site-editing-demo' ) ) {
$block_template_files = array_merge( $block_template_files, glob( dirname( __FILE__ ) . '/demo-block-templates/*.html' ) ?: array() );
}
foreach ( $block_template_files as $path ) {
if ( ! isset( $slug_priorities[ basename( $path, '.html' ) ] ) ) {
continue;
}
$theme_block_template_priority = $slug_priorities[ basename( $path, '.html' ) ];
if (
isset( $theme_block_template_priority ) &&
$theme_block_template_priority < $higher_priority_block_template_priority &&
( empty( $current_template_post ) || $theme_block_template_priority < $slug_priorities[ $current_template_post->post_name ] )
) {
Expand All @@ -183,10 +185,6 @@ function gutenberg_find_template( $template_file ) {
$current_template_post = get_post(
wp_insert_post( $current_template_post )
);

foreach ( parse_blocks( $current_template_post->post_content ) as $block ) {
create_auto_draft_for_template_part_block( $block );
}
} else {
$current_template_post = new WP_Post(
(object) $current_template_post
Expand All @@ -195,6 +193,12 @@ function gutenberg_find_template( $template_file ) {
}

if ( $current_template_post ) {
if ( is_admin() ) {
foreach ( parse_blocks( $current_template_post->post_content ) as $block ) {
create_auto_draft_for_template_part_block( $block );
}
}
$_wp_current_template_id = $current_template_post->ID;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to change the template resolution code in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we need the current template ID saved, and to create template part auto drafts for custom saved templates as well as file ones.

$_wp_current_template_content = $current_template_post->post_content;
}

Expand Down
3 changes: 3 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions packages/edit-site/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@
"@babel/runtime": "^7.4.4",
"@wordpress/block-editor": "file:../block-editor",
"@wordpress/block-library": "file:../block-library",
"@wordpress/blocks": "file:../blocks",
"@wordpress/components": "file:../components",
"@wordpress/core-data": "file:../core-data",
"@wordpress/data": "file:../data",
"@wordpress/editor": "file:../editor",
"@wordpress/element": "file:../element",
"@wordpress/hooks": "file:../hooks",
"@wordpress/i18n": "file:../i18n",
Expand Down
27 changes: 24 additions & 3 deletions packages/edit-site/src/components/block-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { useMemo, useState } from '@wordpress/element';
import { useMemo, useCallback } from '@wordpress/element';
import { uploadMedia } from '@wordpress/media-utils';
import { useEntityProp } from '@wordpress/core-data';
import { parse, serialize } from '@wordpress/blocks';
import {
BlockEditorProvider,
BlockEditorKeyboardShortcuts,
Expand Down Expand Up @@ -39,13 +41,32 @@ export default function BlockEditor( { settings: _settings } ) {
},
};
}, [ canUserCreateMedia, _settings ] );
const [ blocks, setBlocks ] = useState( [] );
const [ content, _setContent ] = useEntityProp(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid shadowing.

I'm replacing this with useEntityBlockEditor in the follow-up PR. Can we ignore it for now?

'postType',
'wp_template',
'content'
);
const initialBlocks = useMemo( () => {
if ( typeof content !== 'function' ) {
const parsedContent = parse( content );
return parsedContent.length ? parsedContent : undefined;
}
}, [] );
const [ blocks = initialBlocks, setBlocks ] = useEntityProp(
'postType',
'wp_template',
'blocks'
);
const setContent = useCallback( ( nextBlocks ) => {
setBlocks( nextBlocks );
_setContent( serialize( nextBlocks ) );
Copy link
Contributor

@youknowriad youknowriad Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to set content (run serialization) on every change? Can't we do something like the editor (use a function edit)

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'm replacing this with useEntityBlockEditor in the follow-up PR. Can we ignore it for now?

}, [] );
return (
<BlockEditorProvider
settings={ settings }
value={ blocks }
onInput={ setBlocks }
onChange={ setBlocks }
onChange={ setContent }
>
<BlockEditorKeyboardShortcuts />
<Sidebar.InspectorFill>
Expand Down
31 changes: 24 additions & 7 deletions packages/edit-site/src/components/editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import {
SlotFillProvider,
DropZoneProvider,
Popover,
navigateRegions,
} from '@wordpress/components';
import { EntityProvider } from '@wordpress/core-data';

/**
* Internal dependencies
Expand All @@ -17,17 +19,32 @@ import Sidebar from '../sidebar';
import BlockEditor from '../block-editor';

function Editor( { settings } ) {
return (
const template = useSelect(
( select ) =>
select( 'core' ).getEntityRecord(
'postType',
'wp_template',
settings.templateId
),
[]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

just noting that the logic in this PR is already available in editPost, we can tell it to just load the template directly. so for now, these packages are still equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are very different.

This doesn't load @wordpress/editor and all of its unnecessary-for-this-context logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ultimately, it might have to load the editor package because there's a lot of useful components there. Save buttons, Editing Post Title (template title)...

I think you're right when you say, they'll be different it's just not there yet.

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 package, yes, but not the provider and/or data flow.

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 package, yes, but not the provider and/or data flow.

return template ? (
<SlotFillProvider>
<DropZoneProvider>
<Notices />
<Header />
<Sidebar />
<BlockEditor settings={ settings } />
<Popover.Slot />
<EntityProvider
kind="postType"
type="wp_template"
id={ settings.templateId }
>
<Notices />
<Header />
<Sidebar />
<BlockEditor settings={ settings } />
<Popover.Slot />
</EntityProvider>
</DropZoneProvider>
</SlotFillProvider>
);
) : null;
}

export default navigateRegions( Editor );
8 changes: 8 additions & 0 deletions packages/edit-site/src/components/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import SaveButton from '../save-button';

export default function Header() {
return (
<div
Expand All @@ -14,6 +19,9 @@ export default function Header() {
<h1 className="edit-site-header__title">
{ __( 'Site Editor' ) } { __( '(beta)' ) }
</h1>
<div className="edit-site-header__actions">
<SaveButton />
</div>
</div>
);
}
4 changes: 4 additions & 0 deletions packages/edit-site/src/components/header/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,7 @@
font-size: 16px;
padding: 0 20px;
}

.edit-site-header__actions {
padding: 0 20px;
}
56 changes: 56 additions & 0 deletions packages/edit-site/src/components/save-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* WordPress dependencies
*/
import { useEntityProp } from '@wordpress/core-data';
import { useEffect, useState, useCallback } from '@wordpress/element';
import { useSelect } from '@wordpress/data';
import { Button } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { EntitiesSavedStates } from '@wordpress/editor';
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we're importing the editor package, just to use this component. What does this component do? I'm wondering whether it belongs to the editor package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the entities saving modal. Maybe it belongs in core-data?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the entities saving modal

What does it do exactly?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it the modal that lists entities with unsaved changes and lets the user choose if they want to update them all at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't it the modal that lists entities with unsaved changes and lets the user choose if they want to update them all at once?

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I guess@editor is the right place though but it makes me think we can reuse more than that from the Editor package (save buttons....) I also thought the components from "editor" had a dependency on EditorProvider. I guess this one is a bit different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it makes me think we can reuse more than that from the Editor package (save buttons....)

Not without refactoring them.

I also thought the components from "editor" had a dependency on EditorProvider. I guess this one is a bit different?

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more here, I feel this component might be misplaced. We could be tempted to update it tomorrow to use the Editor store somehow to enable advanced flows in the editor screen and this will break the Edit Site screen without notice since it's not directly dependent on the editor store aka EditorProvider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we move it to core-data?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe yes, I don't mind if it's done in a separate PR though to think it properly.


export default function SaveButton() {
const [ , setStatus ] = useEntityProp( 'postType', 'wp_template', 'status' );
// Publish template if not done yet.
useEffect( () => setStatus( 'publish' ), [] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why set the status as soon as we render? It's not clear? should this be done on submit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was more natural to write this way and we'll rarely have non-published templates.

We can look at extending EntitiesSavedStates to support additional callbacks.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, this is an error. Someone will call our selectors to check the status for this entity and he'll get "published".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for the edits selectors.


const { isDirty, isSaving } = useSelect( ( select ) => {
const { getEntityRecordChangesByRecord, isSavingEntityRecord } = select(
'core'
);
const entityRecordChangesByRecord = getEntityRecordChangesByRecord();
const changedKinds = Object.keys( entityRecordChangesByRecord );
return {
isDirty: changedKinds.length > 0,
isSaving: changedKinds.some( ( changedKind ) =>
Object.keys(
entityRecordChangesByRecord[ changedKind ]
).some( ( changedName ) =>
Object.keys(
entityRecordChangesByRecord[ changedKind ][ changedName ]
).some( ( changedKey ) =>
isSavingEntityRecord( changedKind, changedName, changedKey )
)
)
),
};
} );
const disabled = ! isDirty || isSaving;

const [ isOpen, setIsOpen ] = useState( false );
const open = useCallback( setIsOpen.bind( null, true ), [] );
const close = useCallback( setIsOpen.bind( null, false ), [] );
return (
<>
<Button
isPrimary
aria-disabled={ disabled }
disabled={ disabled }
isBusy={ isSaving }
onClick={ disabled ? undefined : open }
>
{ __( 'Update' ) }
</Button>
<EntitiesSavedStates isOpen={ isOpen } onRequestClose={ close } />
</>
);
}
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 like that EntitiesSavedStates handle the saving and that this component retrivees the status. It feels like related concerns are separate and most importantly in two separate packages. Not sure how to improve the situation 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.

Maybe EntitiesSavedStates should render the button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or what do you think of this API?

export default function SaveButton() {
	return (
		<EntitiesSavedStates>
			{ ( { isDirty, isSaving, open } ) => {
				const disabled = ! isDirty || isSaving;
				return (
					<Button
						isPrimary
						aria-disabled={ disabled }
						disabled={ disabled }
						isBusy={ isSaving }
						onClick={ disabled ? undefined : open }
					>
						{ __( 'Update Design' ) }
					</Button>
				);
			} }
		</EntitiesSavedStates>
	);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It does seem reasonable but I know we've been talking about rethinking the saving flow (pre/post publish...) regarding FSE so it could have an impact on the API. Maybe we can start as it is right now but not push too far this direction api wise until we're certain of the flows.