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

Block Library: Add Post Content and Title Blocks. #18461

Merged
merged 4 commits into from
Nov 14, 2019

Conversation

epiqueras
Copy link
Contributor

Description

This PR adds the blocks from #17263, limiting the implementation to their front end rendering logic so that they can quickly be merged and used for building block templates without waiting for all the design considerations of #17263.

How has this been tested?

The blocks were added to a template and it was verified that previewing a post renders the template, filling in the placeholders appropriately.

Types of Changes

New Feature: Full Site Editing now has a Post Title and a Post Content Block.

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

*/
function render_block_core_post_content() {
// TODO: Without this temporary fix, an infinite loop can occur.
if ( is_admin() || defined( '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.

The logic of the condition is not clear. maybe expand on the comment. Also if this is temporary what's the ideal fix?

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 It's because in the admin views and in REST requests there is no post loop right? So the rest of this function becomes infinitely recursive?

Is there even a fix for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this should be necessary for all post related blocks too.

For me, this is another reason the Post block is clearer as the condition becomes "is there a parent post context".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't solve this and there is not currently a way for a parent post block to provide context for children during server rendering. You would be relying on the order in which the block tree is traversed while serializing which would be extremely fragile, specially when you start nesting post contexts.

Solving that shouldn't block this PR.

if ( ! in_the_loop() ) {
rewind_posts();
the_post();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that this code is duplicated between post title and post content, As we discussed before, it seems to me that this code is better in a container "post" block. I'm ok moving forward without it and see where the limits of this approach lead us.

That said, we'll probably want to share that code across all post related blocks and avoid duplicating 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.

We'll see. I still think it's valuable to support top level post children blocks like these. The post wrapper block should only be used to overwrite the current post context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract the common code at least?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Nice work here.

@youknowriad
Copy link
Contributor

I don't think it's totally related to this PR but I got this warning while testing this PR (when previewing a post using a template containing these blocks)

Capture d’écran 2019-11-14 à 4 58 03 PM

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Excited to see this land.

@epiqueras
Copy link
Contributor Author

I don't think it's totally related to this PR but I got this warning while testing this PR (when previewing a post using a template containing these blocks)

It's related to block template directory loading.

Excited to see this land.

Me too 🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉!

@epiqueras epiqueras force-pushed the add/post-content-and-title-blocks branch from 32bdf92 to 15eaeab Compare November 14, 2019 19:14
@epiqueras epiqueras merged commit 3ab776b into master Nov 14, 2019
@epiqueras epiqueras deleted the add/post-content-and-title-blocks branch November 14, 2019 19:42
@retrofox
Copy link
Contributor

@epiqueras do you know how this PR was landed despite the tests fail?

@retrofox
Copy link
Contributor

Here the fix: #18543

@epiqueras
Copy link
Contributor Author

Oops sorry, we have occasional timing related transform failures on PRs and I thought that was the case here so I ignored them.

@youknowriad
Copy link
Contributor

it's bad when we can't trust tests :(

@epiqueras
Copy link
Contributor Author

Maybe we should just disable fickle tests for now, since they seem to cause more harm than good.

@@ -74,3 +74,21 @@ function gutenberg_provide_render_callback_with_block_object( $pre_render, $bloc
return apply_filters( 'render_block', $block_content, $block );
}
add_filter( 'pre_render_block', 'gutenberg_provide_render_callback_with_block_object', 10, 2 );

/**
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a separate file for template-related PHP logic? This compat.php file is at high risk for becoming a dumping ground of various utility functions, so should be limited to functions which can be very closely associated with a specific Trac ticket to incorporate a given change. It doesn't seem like we have such a Trac ticket we could link to with this?

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, that would be good. Maybe even template-utils.php.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be good. Maybe even template-utils.php.

Would template.php be sufficient to describe the grouping of this related functionality?

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 it would be confused with templates.php. Maybe a templates directory makes sense at this point? Then, this file could just be called utils.php, templates.php would become create-cpt.php and so on.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I missed that there was a file already. In what way does this function differ that makes it not suitable to be placed in templates.php ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

templates.php just sets up the CPT.

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 utility function for server rendered block implementations that use the post loop.

You could even argue it's not template specific and belongs in compat.php.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Block Suggestion for a new block [Package] Block library /packages/block-library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants