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: Conditionally enqueue blocks scripts when editing wp_template posts #32717

Merged
merged 3 commits into from
May 3, 2019

Conversation

Copons
Copy link
Contributor

@Copons Copons commented May 1, 2019

Changes proposed in this Pull Request

We want to limit FSE blocks to be only used in wp_template posts.
In #32657 we merged a JS solution, as implied by the official docs.

In this alternative approach, we try to only enqueue the blocks scripts (and styles) when editing a wp_template post.

The problem is that the blocks registration must be performed on init (or at least, all Core blocks are registered on init, and I tried to move FSE's to other hooks to no avail), and consequently, the blocks scripts must also be registered on init (before the blocks registration, which automatically enqueues them).
On init, though, we don't have any current_screen information yet, and so wouldn't work:

$current_screen = get_current_screen();
if ( 'wp_template' === $current_screen->post_type ) {
  // wp_register_script( ... )
}

The remaining possibility is to check the query params:

  • $_GET['post_type] is available when creating new posts.
  • get_post_type( $_GET['post'] ) must be used when modifying an existing one.

Testing instructions

  • Check that Content and Template blocks are only available when editing a Template post.
  • Insert a Template block and pick a post to render. Save and preview (or publish and view) the Template post: make sure the Template block is actually rendering its selected post in the front-end.

Fixes Dennis' concerns in #32657 (review) (well, at least tries to 😄)

@Copons Copons added [Type] Question [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 May 1, 2019
@Copons Copons requested review from dmsnell and a team May 1, 2019 11:51
@Copons Copons self-assigned this May 1, 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
Copy link
Contributor Author

Copons commented May 1, 2019

I've noticed the current implementation doesn't work as expected.
Waiting for domReady is perfectly fine for newly inserted blocks, but it also means that, when the editor renders existing blocks, they aren't registered yet, and so the editor marks them as unsupported.

In other words:

  1. Edit a wp_template post containing a core/template block.
  2. The editor parses the post content, and finds the core/template block.
  3. Since it's not registered, the editor marks it as unsupported.
  4. DOM is ready!
  5. We register the core/template block.
  6. We see that the pre-existing core/template block is marked as unsupported.
  7. We can insert a new core/template block.

In other other words: this PR becomes a bugfix. 😓

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.

A couple of nits but this looks good to me

add_action( 'init', array( $this, 'register_blocks' ), 100 );
add_action( 'init', array( $this, 'register_wp_template' ) );

Copy link
Contributor

Choose a reason for hiding this comment

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

The extra space here seems untidy


function is_editor_wp_template() {
$current_screen = get_current_screen();
return 'wp_template' === $current_screen->post_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since $current_screen isn't being used for anything else I'd probably just rewrite this as a one-liner:

return 'wp_template' === get_current_screen()->post_type;

@mattwiebe mattwiebe 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 May 1, 2019
Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

Tests well for me. :shipit:

FWIW I tried the allowed_block_types filter which wasn't useful for this at all, and conditionally registering the script based on post type that will call unregisterBlockType on domReady. Both seemed more convoluted than the approach provided here.

@Copons Copons force-pushed the fix/fse-blocks-conditional-enqueue branch from 5a5aea7 to 2320054 Compare May 3, 2019 11:23
@Copons Copons merged commit 1bb1352 into master May 3, 2019
@Copons Copons deleted the fix/fse-blocks-conditional-enqueue branch May 3, 2019 11:56
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.

4 participants