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

CoBlocks: Correct example image URLs. #37195

Closed
wants to merge 5 commits into from
Closed

Conversation

kwight
Copy link
Contributor

@kwight kwight commented Oct 30, 2019

This PR filters certain CoBlocks settings to correct example image URLs.

Testing instructions

  1. Sandbox the API and widgets.wp.com.
  2. Build the wpcom-block-editor app from this branch, and transfer to your sandbox.
  3. Bust the cache by entering a dummy value for $__autogen_cache_buster_wpcom_block_editor.
  4. Start a new post in the block editor for a sandboxed test site.
  5. Click the inserter in the top control bar, and hover over each of the three affected blocks: Masonry, Stacked, and Logos.
  6. Ensure example images load properly in the preview to the right.

Fixes #37096

@kwight kwight requested review from a team as code owners October 30, 2019 22:41
@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.

@kwight
Copy link
Contributor Author

kwight commented Oct 31, 2019

Server-side changes: D34792-code.

@gwwar gwwar added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] In Progress labels Oct 31, 2019
@kwight
Copy link
Contributor Author

kwight commented Oct 31, 2019

Updated this to no longer rely on server-side changes.

@kwight
Copy link
Contributor Author

kwight commented Oct 31, 2019

This isn't working at the moment; I don't think any blocks are passing the initial checks in getCoBlocksExampleImages.

@kwight
Copy link
Contributor Author

kwight commented Nov 1, 2019

It looks like CoBlocks blocks aren't registered at the time the filter is called; logging every name that goes through getCoBlocksExampleImages seems to list every block but CoBlocks blocks. I wonder if this super late registration on init (priority 99) affects things.

@mmtr
Copy link
Member

mmtr commented Nov 4, 2019

I wonder if this super late registration on init (priority 99) affects things.

I think it's actually this registration, but yeah it's the same problem (scripts enqueued on init rather than on enqueue_block_assets, which is where our wpcom-block-editor/common script gets registered).

@gwwar
Copy link
Contributor

gwwar commented Nov 4, 2019

If it's that much of an issue, we can maybe dispatch to the redux store on dom ready or unstable editor init, but it's not ideal.

@kwight
Copy link
Contributor Author

kwight commented Nov 4, 2019

For comparison, our own FSE plugin register blocks on init with priority 100 (¯_(ツ)_/¯), and the a8c/posts-list block goes through this filter, so there's something deeper going on here – I'll need to keep digging.

@mmtr
Copy link
Member

mmtr commented Nov 5, 2019

For comparison, our own FSE plugin register blocks on init with priority 100 (¯_(ツ)_/¯), and the a8c/posts-list block goes through this filter, so there's something deeper going on here – I'll need to keep digging.

Note that we have dynamic blocks (server-side rendered) and regular blocks (full JS).

Dynamic blocks need to be registered with register_block_type in PHP, and this can be done in the init PHP hook. These blocks don't alter the regular blocks registered with JS, and thus don't trigger the blocks.registerBlockType JS hook. FSE blocks registered on init are dynamic blocks, so this is expected. Actually, a8c/posts-list is not registered there, since that block is not a dynamic block.

On the other hand, regular blocks are registered with registerBlockType in JS, which do trigger the blocks.registerBlockType JS hook. These scripts registering those blocks need to be enqueued on the enqueue_block_editor_assets hook. FSE also does this.

CoBlocks also have dynamic blocks and regular blocks. Dynamic blocks are registered on init, which is correct. However, scripts registering regular JS blocks are enqueued on init, which is incorrect - they should be enqueued on enqueue_block_editor_assets.

@kwight
Copy link
Contributor Author

kwight commented Nov 6, 2019

However, scripts registering regular JS blocks are enqueued on init, which is incorrect - they should be enqueued on enqueue_block_editor_assets.

I just noticed that the script is not even being enqueued there, it's just being registered (although it's obviously enqueued somewhere, because we still get our regular blocks registered). Trying the obvious stuff of switching to wp_enqueue_script, making gutenberg_wpcom_enqueue_scripts on a later priority, and adding coblocks-editor as a dependency for gutenberg-wpcom-common don't seem to make a difference.

@mmtr
Copy link
Member

mmtr commented Nov 14, 2019

Noting that this won't be needed once we activate 1.17.1 on production since the previews use now images hotlinked from Wikimedia Commons.

@kwight
Copy link
Contributor Author

kwight commented Nov 15, 2019

Closing in favour of the upcoming 1.17.1-wpcom – see #37096 (comment).

@kwight kwight closed this Nov 15, 2019
@kwight kwight deleted the fix/coblocks-previews branch November 15, 2019 22:36
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CoBlocks: Logos & Badges, Masonry, Stacked have missing images in Preview
4 participants