Edit Site: Add template loading. #19081

epiqueras · 2019-12-12T01:02:21Z

Follows #19054

Description

This PR continues the edit-site work by loading the front page template into the block editor.

How has this been tested?

It was verified that the site editor now loads the front page template dynamically.

Screenshots

Types of Changes

New Feature: The site editor now loads the front page template.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

youknowriad · 2019-12-12T07:15:51Z

packages/edit-site/src/components/editor/index.js

+				settings.templateId
+			),
+		[]
+	);


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.

They are very different.

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

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.

The package, yes, but not the provider and/or data flow.

lib/edit-site-page.php

youknowriad · 2020-01-09T09:32:27Z

lib/template-loader.php

+				create_auto_draft_for_template_part_block( $block );
+			}
+		}
+		$_wp_current_template_id      = $current_template_post->ID;


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

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

youknowriad · 2020-01-09T09:33:38Z

packages/edit-site/src/components/save-button/index.js

+import { useSelect } from '@wordpress/data';
+import { Button } from '@wordpress/components';
+import { __ } from '@wordpress/i18n';
+import { EntitiesSavedStates } from '@wordpress/editor';


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.

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

It's the entities saving modal

What does it do exactly?

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?

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

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?

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

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.

Can we move it to core-data?

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

youknowriad · 2020-01-10T08:19:56Z

Trying to test this, something I don't understand, why I get "no template found" when I enable the demo templates?

epiqueras · 2020-01-10T11:21:21Z

In the front-end? Does the same happen in add/edit-site-multiple-template-loading?

youknowriad · 2020-01-15T08:07:05Z

lib/edit-site-page.php

+	// Get root template by trigerring `./template-loader.php`'s logic.
+	get_front_page_template();
+	get_index_template();
+	apply_filters( 'template_include', null );


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

$template_id = get_front_page_template();

We need to run the filters so that the template-loader code runs.

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

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.

youknowriad · 2020-01-15T08:08:15Z

packages/edit-site/src/components/block-editor/index.js

@@ -39,13 +41,32 @@ export default function BlockEditor( { settings: _settings } ) {
 			},
 		};
 	}, [ canUserCreateMedia, _settings ] );
-	const [ blocks, setBlocks ] = useState( [] );
+	const [ content, _setContent ] = useEntityProp(


Why underscore?

To avoid shadowing.

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

youknowriad · 2020-01-15T08:09:17Z

packages/edit-site/src/components/block-editor/index.js

+	);
+	const setContent = useCallback( ( nextBlocks ) => {
+		setBlocks( nextBlocks );
+		_setContent( serialize( nextBlocks ) );


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

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

youknowriad · 2020-01-15T08:11:07Z

packages/edit-site/src/components/save-button/index.js

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


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

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.

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

Only for the edits selectors.

youknowriad · 2020-01-15T08:15:12Z

packages/edit-site/src/components/save-button/index.js

+			<EntitiesSavedStates isOpen={ isOpen } onRequestClose={ close } />
+		</>
+	);
+}


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.

Maybe EntitiesSavedStates should render the button?

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> ); }

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.

youknowriad

To be fair, code-wise I'm not totally satisfied with this PR.

I don't like the usage of globals to retrieve the template
I don't like the dependency to the editor package for just a component.
The saving component is a bit convoluted.

That said, I'm all for follow-up PRs to improve the situation (If we can solve the first one in this PR, that would be cool though).

epiqueras

I don't like the usage of globals to retrieve the template

#19081 (comment)

I don't like the dependency to the editor package for just a component.
The saving component is a bit convoluted.

I proposed an alternative, but if I read this correctly:

#19081 (comment)

You are suggesting we wait until we rethink the whole pre/post-publish flow, which I think is reasonable.

epiqueras added the [Feature] Full Site Editing label Dec 12, 2019

epiqueras added this to the Future milestone Dec 12, 2019

epiqueras requested a review from TimothyBJacobs as a code owner December 12, 2019 01:02

epiqueras self-assigned this Dec 12, 2019

epiqueras requested review from youknowriad, mtias and aduth December 12, 2019 01:02

youknowriad reviewed Dec 12, 2019

View reviewed changes

epiqueras force-pushed the add/edit-site-package branch from 0abc8dd to 9590317 Compare December 12, 2019 23:13

epiqueras force-pushed the add/edit-site-template-loading branch from 981c3ec to fa7a8cb Compare December 12, 2019 23:15

epiqueras mentioned this pull request Dec 13, 2019

Edit Site: Add multiple template loading #19141

Merged

6 tasks

epiqueras changed the base branch from add/edit-site-package to master December 17, 2019 13:01

epiqueras force-pushed the add/edit-site-template-loading branch from 042999d to 2e171f9 Compare December 17, 2019 13:04

epiqueras force-pushed the add/edit-site-template-loading branch from 2e171f9 to f52a2f3 Compare January 8, 2020 01:44

mtias reviewed Jan 8, 2020

View reviewed changes

lib/edit-site-page.php Show resolved Hide resolved

youknowriad reviewed Jan 9, 2020

View reviewed changes

epiqueras added 6 commits January 9, 2020 14:50

Edit Site: Add root template ID to editor settings.

77434ae

Edit Site: Load root template in block editor.

7380c69

Lib: Update demo block template.

2180025

Lib: Rename demo block template.

0938cab

Edit Site: Serialize blocks on persistent changes.

82fe02f

Edit Site: Add multiple entity saving functionality.

7f8488c

epiqueras force-pushed the add/edit-site-template-loading branch from f52a2f3 to 7f8488c Compare January 9, 2020 19:51

youknowriad reviewed Jan 15, 2020

View reviewed changes

youknowriad approved these changes Jan 16, 2020

View reviewed changes

epiqueras commented Jan 16, 2020

View reviewed changes

epiqueras merged commit e33a49f into master Jan 16, 2020

mtias deleted the add/edit-site-template-loading branch January 16, 2020 18:06

ellatrix modified the milestones: Future, Gutenberg 7.3 Jan 20, 2020

Edit Site: Add template loading. #19081

Edit Site: Add template loading. #19081

Conversation

epiqueras commented Dec 12, 2019

Description

How has this been tested?

Screenshots

Types of Changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jan 10, 2020

epiqueras commented Jan 10, 2020

youknowriad Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

epiqueras left a comment

Choose a reason for hiding this comment

youknowriad Jan 15, 2020 •

edited

Loading

youknowriad Jan 15, 2020 •

edited

Loading