close
The Wayback Machine - https://web.archive.org/web/20191209032231/https://github.com/WordPress/gutenberg/pull/18461
Skip to content

/gutenberg

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
+230 −2

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 12, 2019

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. .
@epiqueras epiqueras added this to the Future milestone Nov 12, 2019
@epiqueras epiqueras self-assigned this Nov 12, 2019
@epiqueras epiqueras mentioned this pull request Nov 12, 2019
Closed
5 of 5 tasks complete
@youknowriad
youknowriad reviewed Nov 13, 2019
View changes
packages/block-library/src/post-content/index.php Outdated
*/
function render_block_core_post_content() {
// TODO: Without this temporary fix, an infinite loop can occur.
if ( is_admin() || defined( 'REST_REQUEST' ) ) {

This comment has been minimized.

Sign in to view
Copy link
@youknowriad

youknowriad Nov 13, 2019

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@epiqueras

epiqueras Nov 13, 2019

Author Contributor

@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?

This comment has been minimized.

Sign in to view
Copy link
@youknowriad

youknowriad Nov 13, 2019

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@epiqueras

epiqueras Nov 13, 2019

Author Contributor

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.
@youknowriad
youknowriad reviewed Nov 13, 2019
View changes
packages/block-library/src/post-content/index.php Outdated
if ( ! in_the_loop() ) {
rewind_posts();
the_post();
}

This comment has been minimized.

Sign in to view
Copy link
@youknowriad

youknowriad Nov 13, 2019

Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@epiqueras

epiqueras Nov 13, 2019

Author Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@youknowriad

youknowriad Nov 14, 2019

Contributor

Can we extract the common code at least?

This comment has been minimized.

Sign in to view
Copy link
@epiqueras

epiqueras Nov 14, 2019

Author Contributor

d01218b
@youknowriad
youknowriad reviewed Nov 13, 2019
View changes
Copy link
Contributor

youknowriad left a comment

Nice work here.
@youknowriad

This comment has been minimized.

Sign in to view
Copy link
Contributor

youknowriad commented Nov 14, 2019

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
@youknowriad
youknowriad approved these changes Nov 14, 2019
View changes
Copy link
Contributor

youknowriad left a comment

Excited to see this land.
@epiqueras

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

epiqueras commented Nov 14, 2019

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 requested review from nerrad and ntwb as code owners Nov 14, 2019
epiqueras added 4 commits Nov 12, 2019
@epiqueras
Block Library: Add Post Content and Title Blocks.
4800408
@epiqueras
Block Library: Factor out common post loop handling code.
3133477
@epiqueras
Block Library: Make post object handling explicit.
cea3600
@epiqueras
Fixtures
Loading status checks…
15eaeab
@epiqueras epiqueras force-pushed the add/post-content-and-title-blocks branch from 32bdf92 to 15eaeab Nov 14, 2019
@epiqueras epiqueras merged commit 3ab776b into master Nov 14, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Failed
Details
@epiqueras epiqueras added this to Done in Phase 2 via automation Nov 14, 2019
@epiqueras epiqueras deleted the add/post-content-and-title-blocks branch Nov 14, 2019
@retrofox

This comment has been minimized.

Sign in to view
Copy link
Contributor

retrofox commented Nov 15, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

retrofox commented Nov 15, 2019

Here the fix: #18543
@epiqueras

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

epiqueras commented Nov 15, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

youknowriad commented Nov 15, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

epiqueras commented Nov 15, 2019

Maybe we should just disable fickle tests for now, since they seem to cause more harm than good.
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.0 Nov 25, 2019
@apeatling apeatling mentioned this pull request Nov 29, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad

@talldan talldan

@Soean Soean

@ntwb ntwb

@ajitbohra ajitbohra

@nerrad nerrad

@TimothyBJacobs TimothyBJacobs

@jorgefilipecosta jorgefilipecosta

Assignees

@epiqueras epiqueras

Labels
[Feature] Full Site Editing [Package] Block library
Projects
Phase 2
  
Done
Milestone
  Gutenberg 7.0
3 participants
@epiqueras @youknowriad @retrofox
You can’t perform that action at this time.