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: Add a Template Part block #32581

Merged
merged 19 commits into from
Apr 29, 2019

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Apr 25, 2019

Changes proposed in this Pull Request

Note: this is based off #32529, so it will need to be rebased when that's merged.

  • Add a Template Part block.
    It roughly works like the Content Slot block but, whereas that's a sort of placeholder for the template to be filled with a post or page, the Template Part is used to render a layout element into the template.

The screenshot is basically the same (of course with a different title and label):

Apr-23-2019 18-47-49

The main visible difference is that this renders the selected post's content on the front-end.

Testing instructions

  • Follow the README to build the plugin. (If the script fails, please run npm distclean and npm ci first).
  • In the block editor, try adding the Template Part block.
  • Type a post name (one with content 🙂) and select it.
  • Make sure its content shows up in the editor.
  • Save and preview (or view) the post.
  • Make sure the preview contains the selected post's content.

Fixes #32493

@Copons Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Full Site Editing labels Apr 25, 2019
@Copons Copons requested a review from a team April 25, 2019 15:19
@Copons Copons self-assigned this Apr 25, 2019
@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.

@Copons Copons force-pushed the update/32492-fse-content-preview-block branch from f6d8ab7 to 0c206af Compare April 25, 2019 16:12
@Copons Copons force-pushed the add/32493-fse-layout-renderer-block branch from c0680e1 to e78ec79 Compare April 25, 2019 16:31
@Copons Copons force-pushed the update/32492-fse-content-preview-block branch from 0c206af to 965fdb4 Compare April 25, 2019 16:44
@Copons Copons force-pushed the add/32493-fse-layout-renderer-block branch from e78ec79 to 03c2638 Compare April 25, 2019 16:47
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.

Looks solid overall - left a few small change requests

} );
};

const edit = withState( {
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 conventional in core for a block's edit method to live in its own file. We should probably try to follow those when needed for future core contribution possibilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point.
I'll also update the Content Preview block accordingly.

if ( ! $attributes['selectedPostId'] || ! is_int( $attributes['selectedPostId'] ) ) {
return;
}
$align = $attributes['align'] ? ' align' . $attributes['selectedPostId'] : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs an isset check otherwise we get undefined index warnings

$align = $attributes['align'] ? ' align' . $attributes['selectedPostId'] : '';
$post = get_post( $attributes['selectedPostId'] );
setup_postdata( $post );
$content = '<div class="a8c-content-renderer'. $align . '">' . get_the_content() . '</div>';
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to do apply_filters( 'the_content', get_the_content() ) so we get do_blocks and other expected formatting. Try rendering a post with a dynamic block (eg Latest Posts) in the post you're rendering and it won't render right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn this has also fixed an error I was struggling to figure out.

Post X contains a renderer rendering post Y which contains another renderer for post Z.
On the front-end, post X shows Y correctly, but not Z.
This was because I wasn't do_blocks-ing Y!

TWISTED! 🧶

Copy link
Member

Choose a reason for hiding this comment

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

To extend this a bit further, we can also create circular dependencies now. For example: Post X contains a renderer for Post Y, and Post Y contains a rendered for Post X. When you try to save this the updating will fail, and if you try to reload the editor it will get into an infinite loop (Allowed memory size of 268435456 bytes exhausted (tried to allocate 262144 bytes) in /var/www/html/wp-includes/plugin.php on line 465)

Anyway, this is and edge case that we can tackle in follow-up PRs. 😄

@mattwiebe
Copy link
Contributor

I'm a bit rusty with the PHP customs, and the require on top of the file feels a bit off to me.
Do you think it's good enough, or should I spelunk WPCOM in search of some consistent approach?

Top of the file is pretty normal when it's something that's always going to be required. The block will need to be registered on every load so it's fine. When it's more discrete functionality, we usually load it inside the function it will be used in.

@vindl
Copy link
Member

vindl commented Apr 26, 2019

A couple of PHP warnings are dumped to the top of body when I reload the editor with this block saved in the content:

Notice: Undefined index: align in /var/www/html/wp-content/plugins/full-site-editing/blocks/content-renderer/index.php on line 7

Warning: Cannot modify header information - headers already sent by (output started at /var/www/html/wp-content/plugins/full-site-editing/blocks/content-renderer/index.php:7) in /var/www/html/wp-includes/option.php on line 947

Warning: Cannot modify header information - headers already sent by (output started at /var/www/html/wp-content/plugins/full-site-editing/blocks/content-renderer/index.php:7) in /var/www/html/wp-includes/option.php on line 948

instructions={ __( 'Select something to render' ) }
>
<div className="a8c-content-renderer-block__selector">
<PostAutocomplete
Copy link
Member

@vindl vindl Apr 26, 2019

Choose a reason for hiding this comment

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

Should we limit the results here to wp_template CPTs? I know this hasn't been merged yet but just wanted to check whether you think this is something that we should mark as a future todo item.

To continue this line of reasoning a bit, what do you all think about imposing the following constraints initially:

  • we should allow inserting both Content Preview and Content Renderer blocks only in wp_template CPT context. The former will serve as a placeholder post_content for the post that consumes the given template, and the second as a method for reusing template parts (e.g. header, footer) across multiple templates.
  • wp_template can contain only one Content Preview block (we'll permit rendering only one post within a template for now; no mixing of content and template parts).
  • Content Preview block will only show pages in its preview suggestions (or more broadly, not show wp_template CPTs).
  • Content Renderer block will show only wp_template CPTs in its post suggestions.

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 think that's something we might want to discuss more extensively.
On one hand, this block intent is to render layout elements.
On the other, what if I want to render a post inside a template?

A simple example that comes to mind would be a dynamic piece of content in the sidebar that wouldn't require me to update the entire template to change. (basically a widget of some sort).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per the next steps (I think we can tackle 3 out of 4 in follow ups, and the remaining in the other PR):

  • we should allow inserting both Content Preview and Content Renderer blocks only in wp_template CPT context. The former will serve as a placeholder post_content for the post that consumes the given template, and the second as a method for reusing template parts (e.g. header, footer) across multiple templates.

This makes sense, at least initially.
But then, I'm up for max flexibility, and so I'd be ok letting folks inserting a Renderer inside any post type.
(Thinking about it, Preview wouldn't make much sense outside a Template.

  • wp_template can contain only one Content Preview block (we'll permit rendering only one post within a template for now; no mixing of content and template parts).

Yes indeed! This is a 1-liner change: just add multiple: false to the block support properties. (I will actually do this in #32529 asap. No need to open an whole new PR for this).

  • Content Preview block will only show pages in its preview suggestions (or more broadly, not show wp_template CPTs).

Ok for not including Templates, but I'd leave it open to any kind of (publicly searchable) CPT.
Say, for a Woo product, or a JP portfolio page, etc.

  • Content Renderer block will show only wp_template CPTs in its post suggestions.

Ok, but see my previous comment. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

A simple example that comes to mind would be a dynamic piece of content in the sidebar that wouldn't require me to update the entire template to change. (basically a widget of some sort).

Sounds like something that better fits a reusable block use-case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unless you also want to display it as a normal post. 😄

I mean, folks use WP for the weirdest thing, and I'd love to keep giving them such freedom.
You're definitely right in scoping this down, just, let's not exclude this completely for future iterations.

@vindl
Copy link
Member

vindl commented Apr 26, 2019

Nice work @Copons! I left a few comments, but their purpose is mainly to chart out some future work, and I would consider them non-blocking (except maybe for #32581 (comment) if it can be resolved quickly).

@Copons Copons added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 26, 2019
@Copons Copons force-pushed the add/32493-fse-layout-renderer-block branch 2 times, most recently from 69e11bc to 5073d86 Compare April 26, 2019 18:23
@Copons Copons changed the title Full Site Editing: Add a Content Renderer block Full Site Editing: Add a Template Part block Apr 26, 2019
@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 Apr 26, 2019
@vindl vindl added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 26, 2019
@vindl vindl dismissed mattwiebe’s stale review April 26, 2019 19:28

Comments resolved.

@Copons Copons force-pushed the add/32493-fse-layout-renderer-block branch from 5073d86 to a49bc53 Compare April 29, 2019 10:36
@Copons Copons changed the base branch from update/32492-fse-content-preview-block to master April 29, 2019 10:37
@Copons Copons merged commit ec422ed into master Apr 29, 2019
@Copons Copons deleted the add/32493-fse-layout-renderer-block branch April 29, 2019 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full Site Editing: create page_renderer block [4]
4 participants