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

Full Site Editing: Theme and Front-end Rendering #32865

Closed
wants to merge 14 commits into from

Conversation

Copons
Copy link
Contributor

@Copons Copons commented May 7, 2019

WARNING

This is a prototype! The code is rough!
It's mostly intended to be installed and tested.
Hopefully there are no infinite loops anymore, but you never know.

Changes proposed in this Pull Request

Template Selection

Users can choose the template for Posts and Pages through the new Template sidebar.
It contains a wp_template_id meta box with an autocomplete input searching for Templates.

Notes:

  • The Template sidebar should be available for all viewable CPTs, not just Posts and Pages.
  • The wp_template_id autocomplete should only search for Templates containing a Content Slot block.
  • The meta box should be clearable in order to use the default template.

Template Hierarchy

Add barebone and very rough support for template hierarchy.
Think of it like an abstraction/simplification of the Core hierarchy.

Users can assign a Template to a "screen" (for lack of a better term) through the new Template Hierarchy sidebar.
It contains a list of checkbox for all the screens that could use a template.
Currently:

  • index: the default fallback template for all screens.
  • singular: the default for is_singular() screens.
  • All viewable post types (e.g. post, page, media): the default for non-singular screens of the given post type (e.g. archives, or posts list page).

Blank Theme

Add a very minimalistic Blank Theme, which works very roughly like the Core template selection.
It has two behaviours, depending if the screen is_singular or otherwise.

Singular screens are straightforward: render the first available template; then the Content Slot block inside it will render the post content.
The template selection order for singular screens is:

  • Post's own wp_template_id.
  • $template_hierarchy->singular.
  • $template_hierarchy->index.

Non-singular screens are a bit more complicated: select the first available template; then the Content Slot block inside it will render the entire posts list.
The template selection order for non-singular screens is:

  • $template_hierarchy->$post_type.
  • $template_hierarchy->index.

Notes:

  • This implementation means that Content Slot is responsible of the HTML formatting of the list. This is not ideal imho, but I don't have a better idea right now.
  • In the future we should probably give the users the possibility of creating a template for list items (e.g. a template with "slot blocks" for post title, excerpt, comments count, etc.).

Screenshots

Template List Homepage Template
Screenshot 2019-05-07 at 18 33 10 Screenshot 2019-05-07 at 18 33 45
Sample Page (Editor) Sample Page (Rendered)
Screenshot 2019-05-08 at 17 15 00 Screenshot 2019-05-07 at 18 34 27
Template Hierarchy Sidebar Posts List (Rendered)
Screenshot 2019-05-10 at 16 59 36 Screenshot 2019-05-10 at 16 59 54

Notes:

  • This PR does not contain any CSS, not even obtained from Twenty Nineteen.
  • The fake navigation menu is just a column block with three links inside. For some reason, it's not displayed as flex in the editor, and so it awkwardly wraps the columns when rendered in preview, but looks just fine when rendered in the front-end.

Testing instructions

  • Follow the FSE readme, and setup both the plugin and the theme.
  • In the test WP, enable both the FSE plugin and the Blank theme.
  • Create a Template containing some blocks (or Template Parts of other Templates), and a Content Slot block.
  • Create or edit a page or a post, open the Template sidebar, and select the template created earlier.
  • Save and view the page.
  • It should display the whole template, with the post content instead of the Content Slot block.
  • Edit the template, open the Template Hierarchy sidebar, and check "Default".
  • Open the posts page of the site.
  • It should display the whole template, with the posts list (just linked title and content, and without pagination) instead of the Content Slot block.

Fixes #32496
Fixes #32617

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@mattwiebe mattwiebe left a comment

Choose a reason for hiding this comment

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

Found one big problem and a couple of smaller things. I also found that my page wasn't rendering properly on the front end (nothing came out of render_post_content_block). I can help dig into that later but this has enough problems for me that I'm going to stop for now

get_header();

$post_id = get_the_ID();
$template_id = get_post_meta( $post_id, 'wp_template_id', true );
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use a leading underscore on this meta like core does for choosing templates: _wp_page_template. Any meta with a leading underscore is hidden by default from the Custom Fields metabox so that users can't mess them up.

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 might be messing up something else, but if I register _wp_template_id, it doesn't let me update it via REST:

{
  "code": "rest_cannot_update",
  "message": "Sorry, you are not allowed to edit the _wp_template_id custom field.",
  "data": {
    "key": "_wp_template_id",
    "status": 403
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This (untested) should address that issue:

register_post_meta( '', '_wp_template_id', array( 'show_in_rest' => true );

$template_id = get_post_meta( $post_id, 'wp_template_id', true );
$template = get_post( $template_id );

if ( $template_id !== $post_id && $template ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably also add a post type check, like 'wp_template' === $template->post_type here. I would not be surprised if we wind up wanting something like a is_template() function that will accept an ID or WP_Post object.

@Copons Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels May 10, 2019
);
} );

if ( 'wp_template' === fullSiteEditing.editorPostType ) {
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 think the component file is the right place for this. I get it (don't like it either) with blocks, but I expect components to export. How do we feel about moving this (and import { registerPlugin } from '@wordpress/plugins';) to full-site-editing-plugin/index.js ??

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've thought about it, and tbh I might even go as far as creating a new folder (plugins?), and keep the index as clean as possible.

);
} );

if ( 'wp_template' !== fullSiteEditing.editorPostType ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

}
}

$template = get_post( $template_id );
Copy link
Contributor

Choose a reason for hiding this comment

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

we need something here for post types with no $template_id. On my clean install, I tried

$template = get_post( $template_id ?? null );

yeah, null-coalescing operator because PHP 7 has it and I won't listen otherwise.

Before After
before after

@mattwiebe
Copy link
Contributor

mattwiebe commented May 13, 2019

EDIT: nevermind, hadn't brought in the latest changes. The bottom paragraph may still may be worth considering.

(Tried this a bit again and I'm still getting infinite loops when 1) saving a wp_template or 2) trying to load a wp_template post for editing. I'll keep digging in to try to understand why it's happening.)

As a semi-aside, I think we're going to need some kind of dedicated function for rendering templates and/or post content that keeps track of the stack that's been rendered so far so as to try to prevent nesting the same template, and also we probably need to go away from just calling apply_filters( 'the_content', $content ) and instead explicitly call the functions hooked to it (do_blocks, wptexturize, wpautop, etc) to also avoid this.

@gwwar
Copy link
Contributor

gwwar commented May 14, 2019

Tried this a bit again and I'm still getting infinite loops

Being able to detect a cycle here would be lovely, but for now let's add a max depth for rendering before we give up. For our initial use case limiting this to say a depth of 10 would probably be more than enough, and we can add logging in this case to help figure out what's happening.

Edit: Thinking on this more, do we need more than a depth of two? The page template will need a content slot, and may contain template parts. Any other template parts, probably don't need to nest further. I think it's fine to constrain this, and revisit later if we find a use case.

@@ -1,11 +1,37 @@
<?php

function render_post_content_block( $attributes, $content ) {
// Early return to avoid infinite loops in the REST API
if ( is_admin() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity what does the trace look like? Are there similar rendering conditions we need to be aware of?

@Copons
Copy link
Contributor Author

Copons commented May 14, 2019

This PR was opened to back my exploration on FSE front end rendering (paAmJe-lZ-p2), and so it's purpose is now complete.
I'll close it and start tackling single features in their own smaller PRs, but feel free to keep discussing here on project-wide topics.

do we need more than a depth of two?

@gwwar We probably don't, but I don't see a reason why we can't.
"Template parts containing template parts" doesn't sound so far-fetched!
At the same time, I agree we should put a limit to the nesting. 5? 10? From a code standpoint, any sounds good to me.

Out of curiosity what does the trace look like?

I'm not sure if @mattwiebe got the same, but in my case the Content Slot render callback ran out of memory, while also being called continuously by the element re-rendering in React (at least, this is my suspicion).

Now it should be fixed (and also @mattwiebe said that he wasn't running the latest commit), but we totally need to keep an eye out for infinite loops, because in FSE they might happen in the most unexpected ways (for example, I didn't consider REST requests!).

@Copons Copons closed this May 14, 2019
@Copons Copons deleted the try/fse-blank-theme branch May 14, 2019 15:05
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 14, 2019
@vindl vindl restored the try/fse-blank-theme branch May 15, 2019 04:11
@vindl vindl deleted the try/fse-blank-theme branch May 15, 2019 04:15
@Copons Copons restored the try/fse-blank-theme branch December 3, 2019 18:06
@vindl vindl deleted the try/fse-blank-theme branch March 6, 2020 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants