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

Edit Site: Add template loading. #19081

Merged
merged 6 commits into from Jan 16, 2020
Merged

Edit Site: Add template loading. #19081

merged 6 commits into from Jan 16, 2020

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Dec 12, 2019

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

Screen Shot 2019-12-11 at 4 55 42 PM

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. .
@epiqueras epiqueras added this to the Future milestone Dec 12, 2019
@epiqueras epiqueras requested a review from TimothyBJacobs as a code owner Dec 12, 2019
@epiqueras epiqueras self-assigned this Dec 12, 2019
@epiqueras epiqueras requested review from youknowriad, mtias and aduth Dec 12, 2019
settings.templateId
),
[]
);

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 12, 2019

Author Contributor

They are very different.

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 13, 2019

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 13, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 13, 2019

Author Contributor

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

@epiqueras epiqueras force-pushed the add/edit-site-package branch from 0abc8dd to 9590317 Dec 12, 2019
@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from 981c3ec to fa7a8cb Dec 12, 2019
@epiqueras epiqueras mentioned this pull request Dec 13, 2019
5 of 6 tasks complete
@epiqueras epiqueras changed the base branch from add/edit-site-package to master Dec 17, 2019
@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from 042999d to 2e171f9 Dec 17, 2019
@karmatosed karmatosed added this to Inbox in Full site editing Dec 19, 2019
@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from 2e171f9 to f52a2f3 Jan 8, 2020
create_auto_draft_for_template_part_block( $block );
}
}
$_wp_current_template_id = $current_template_post->ID;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 9, 2020

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 9, 2020

Author Contributor

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

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 9, 2020

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 9, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 9, 2020

Contributor

It's the entities saving modal

What does it do exactly?

This comment has been minimized.

Copy link
@mtias

mtias Jan 9, 2020

Contributor

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?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 9, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 9, 2020

Contributor

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?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 9, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 10, 2020

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 10, 2020

Author Contributor

Can we move it to core-data?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 10, 2020

Contributor

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

@epiqueras epiqueras force-pushed the add/edit-site-template-loading branch from f52a2f3 to 7f8488c Jan 9, 2020
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 10, 2020

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

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Jan 10, 2020

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

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 15, 2020

Contributor

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

$template_id = get_front_page_template();

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 15, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 16, 2020

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 16, 2020

Author Contributor

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.

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 15, 2020

Contributor

Why underscore?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 15, 2020

Author Contributor

To avoid shadowing.

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

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 15, 2020

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 15, 2020

Author Contributor

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

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 15, 2020

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 15, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 16, 2020

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 16, 2020

Author Contributor

Only for the edits selectors.

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 15, 2020

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 15, 2020

Author Contributor

Maybe EntitiesSavedStates should render the button?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 15, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 16, 2020

Contributor

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.

Copy link
Contributor

youknowriad left a comment

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).

Copy link
Contributor Author

epiqueras left a comment

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 epiqueras merged commit e33a49f into master Jan 16, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@mtias mtias deleted the add/edit-site-template-loading branch Jan 16, 2020
@ellatrix ellatrix modified the milestones: Future, Gutenberg 7.3 Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.