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

Editor: Introduce a new dynamic templates mode and add basic post title and content blocks. #17263

Closed
wants to merge 12 commits into from

Conversation

epiqueras
Copy link
Contributor

@epiqueras epiqueras commented Aug 29, 2019

Depends on #17207

Description

This PR implements a new custom post type for implementing a dynamic template hierarchy like in #4659. Then it makes a few changes to the editor store to load in a "template mode" when it detects that the current post has an attached template.

It also brings in the post title and post content blocks of #16565, but using the new declarative APIs introduced in #17153 and #17207.

All of this new functionality is enabled only when you enable the full site editing experiment in the experiments tab. This way we can iterate it on it, until it's ship-able.

If you try the experiment on this branch, you should see posts load in template mode where you can edit the top level template, the post title and content blocks, preview the post, etc, etc. Blocks have their own save buttons for now, until we design all the different save flows we should have and how they should interact with each other.

How has this been tested?

It hasn't 😬

Screenshots

Screen Shot 2019-08-29 at 4 14 04 PM

Types of Changes

New Feature: Introduce a new dynamic templates mode and add basic post title and content blocks.

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.

@epiqueras epiqueras added [Package] Core data /packages/core-data [Package] Editor /packages/editor [Package] Block library /packages/block-library [Package] Block editor /packages/block-editor [Package] Edit Post /packages/edit-post labels Aug 29, 2019
@epiqueras epiqueras added this to the Future milestone Aug 29, 2019
@epiqueras epiqueras self-assigned this Aug 29, 2019
function render_block_core_post_title() {
rewind_posts();
the_post();
return the_title( '<h1>', '</h1>', false );
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the heading level be variable? In a singular context, it should be h1. But on an archive context where there is more than one post in the loop, should it not be h2?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should behave like the Heading block where you can pick from among the heading levels. There could be an “auto” setting which does this, but which you could also override as desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this should behave like the Heading block where you can pick from among the heading levels. There could be an “auto” setting which does this, but which you could also override as desired.

Yeah I like that.

This was more of a first step to get something on the screen. We need a major design effort to come up with how these new blocks will behave. See the questions at the end of this overview issue: #17284

lib/templates.php Outdated Show resolved Hide resolved
array(
'post_type' => 'wp_template',
'post_name' => 'single-post',
'post_content' => '<!-- wp:post-title /--><!-- wp:post-content /-->',
Copy link
Member

Choose a reason for hiding this comment

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

Should the featured image be factored in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's something we want in the default "post" post type template, sure.

This shouldn't be in Core or the plugin though. We should provide the framework and UI for users to create new templates, but default templates, if there are any, should be provided by themes.

$template_query = new WP_Query(
array(
'post_type' => 'wp_template',
'name' => 'single-post',
Copy link
Member

Choose a reason for hiding this comment

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

Naturally, this logic will need to be fleshed out with looking up the specific template for a given post. For example, if editing a movie post type, it should look for single-movie before single. And if single does not exist, then singular, and so on.

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, see #4659

*/
function render_block_core_post_content() {
rewind_posts();
the_post();
Copy link
Member

Choose a reason for hiding this comment

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

Should the post not be provided as an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, this block renders whatever post its context dictates.

We do need to add support for pages with multiple posts/post blocks.

function render_block_core_post_content() {
rewind_posts();
the_post();
return apply_filters( 'the_content', str_replace( ']]>', ']]&gt;', get_the_content() ) );
Copy link
Member

Choose a reason for hiding this comment

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

Should this not receive a wrapping element (like many themes put a div.entry-content around it)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've seen that, but I'm not very familiar with everything it's used for.

Is that something we should hardcode or expect themes to add it in a filter?

Copy link
Member

Choose a reason for hiding this comment

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

I think we would need to align on a wrapping div with a certain class name. I assume based on what I've seen, one of the most common classes used for this is entry-content. An alternative would be to use a typical block class, e.g. wp-block-post-content.

This leads to a broader discussion on how the class naming scheme of Gutenberg should evolve when it comes to entire templates. Is the wp-block-... naming sufficient, should it expand to template blocks as well? Or should we constrain the typical block classes to in-content blocks and define a set of more traditional class names?

Finding a consistent solution here is crucial for themes styling Gutenberg templates.

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'll probably have to use entry-content for backwards compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

I think that depends on whether we can get the approach in its entirety to be backward-compatible or not. I think a site-wide block based editing experience will require massive changes in how themes are developed either way. It shouldn't break existing themes, but I think certain features would just not be supported with an old theme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but keeping it as entry-content wouldn't hurt.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, any existing class names from Microformats should be preserved, as they relate to semantics. And entry-content is one such example from hAtom. Use of such legacy Microformats should not preclude using Schema.org microdata.

onInput={ useCallback( () => {
setContent( ( { blocks: blocksForSerialization = [] } ) =>
serializeBlocks( blocksForSerialization )
);
Copy link
Member

Choose a reason for hiding this comment

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

Not a concern with this code, but I'm curious how this would translate to an archive context. It absolutely makes sense to start with the single post context for this exploration, and in that context combining editing of the template blocks and post blocks should work fine. But once we get to a place where there are multiple posts in one page (and the list would change over time), the UI might fill up quite a bit. This could be reduced by some kind of "zoom-in" mode where post content is mostly hidden and can be expanded on click and/or with a link to "edit this specific post" (I believe this was considered before somewhere). Another consideration might be performance - having blocks from let's say 10+ posts (plus the template blocks) in one view could be a lot, especially when assuming most people would only use the archive view for editing the template, but not individual posts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if the editor is not operating on a single post, the post block can easily infer that from useEntityId returning undefined, and bail out of loading anything, showing a placeholder instead.

const saveProps = [ 'title', 'slug' ];
export default function PostTitleEdit() {
const [ title, setTitle ] = useEntityProp( 'postType', 'post', 'title' );
const [ , setSlug ] = useEntityProp( 'postType', 'post', 'slug' );
Copy link
Member

Choose a reason for hiding this comment

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

The hard-coding of post here is probably just temporary, but we need to figure out a way to do it right, since currently this only works for posts of post type 'post' and causes an error on other post types.

The problem is that the overall current post being edited appears to be overridden by this implementation, causing getCurrentPost() to return the wp_template instead of the actual post being edited. Otherwise we could have used withSelect() to get the current post type, but that wouldn't work.

I'm a bit wary of overriding the current post context in Gutenberg to become a wp_template post when actually editing a "real" post, since it could break expectations by other plugins. We should probably think about a way we expose the current wp_template through a set of new API functions (e.g. getCurrentTemplate() etc.), where we could transform the editor accordingly based on whether these include a wp_template post or not (if not, it would be just like today).

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 editor already supports editing multiple post types and the editor store has a getPostType selector.

Copy link
Member

Choose a reason for hiding this comment

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

Are you referring to getCurrentPostType()? It still returns wp_template here when I use it. How do I get the post type of the actual post being edited? It's hard-coded to post here, so we need to replace it with a function that dynamically gets the correct post type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I thought you were asking how to get the post type of the template. (The template is the actual post being edited after all.)

You can get the post that uses the template through editor settings:

const { postId, postType, templateId } = getEditorSettings()

@felixarntz
Copy link
Member

@epiqueras I made two more small changes:

  • Wrap core/post-content into div.entry-content as discussed.
  • Put in a (temporary) fix for a possible infinite loop. This is not a proper solution, and we might need to find a better way to fix it. I haven't been able to dig to its roots, but basically the block parser continues to iterate the wp_template endlessly when processing the content in a REST API request (which also happens during admin initialization because of preloading).

@@ -11,6 +11,11 @@
* @return string Returns the filtered post content of the current post.
*/
function render_block_core_post_content() {
// TODO: Without this temporary fix, an infinite loop can occur.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's add inline comments as to how this fixes the infinite loop and why it occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixarntz

Put in a (temporary) fix for a possible infinite loop. This is not a proper solution, and we might need to find a better way to fix it. I haven't been able to dig to its roots, but basically the block parser continues to iterate the wp_template endlessly when processing the content in a REST API request (which also happens during admin initialization because of preloading).

Interesting, are you sure you didn't have nested post content blocks?

What are the repercussions of this fix. Will returning an empty string like this affect anything? If not, I guess we can keep it.

@epiqueras epiqueras changed the base branch from add/site-title-block to master October 24, 2019 00:04
@epiqueras
Copy link
Contributor Author

It's all coming together:

https://youtu.be/HXClt6oNtcg

@youknowriad @felixarntz

@youknowriad
Copy link
Contributor

@epiqueras Can we extract the post title and post content blocks from this PR into their own PR. These could be useful on their own to build templates in the temporary UI.

@epiqueras
Copy link
Contributor Author

They can't really be tested without a template mode.

@youknowriad
Copy link
Contributor

@epiqueras How so, I mean their "edit" version can't but their "placeholder" version and frontend rendering are very useful even without the "template mode"

@epiqueras
Copy link
Contributor Author

Most of their code is the edit part though, and it would be strange to merge that in a PR where you can't test it and it could be completely broken.

@youknowriad
Copy link
Contributor

I get that and maybe we can leave that part for this PR. That said, in several occasions, I found myself trying to see if I can build a block-based template and I don't really need the "edit" functions for this. I feel these two blocks are the missing pieces to achieve a first raw block-based theme.

@epiqueras
Copy link
Contributor Author

I can open a PR for that, but I'm worried that it will just encourage this one to go stale.

Unless, your idea is to postpone any sort of "mode" switching in the near future.

@youknowriad
Copy link
Contributor

I think this one has still a lot of design considerations to be taken care of before landing while the other one seems easier. Also I think since it reduces the changes from this PR to the template mode, it will be easier to review and land.

@epiqueras
Copy link
Contributor Author

@youknowriad that's fair. Here: #18461.

@epiqueras
Copy link
Contributor Author

Closing this as it will be merged in more granular PRs.

@epiqueras epiqueras closed this Nov 20, 2019
@youknowriad youknowriad deleted the add/template-and-post-blocks branch November 21, 2019 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Package] Block library /packages/block-library [Package] Core data /packages/core-data [Package] Edit Post /packages/edit-post [Package] Editor /packages/editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants