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

Standalone Block Registration/Block Dependencies #10503

Merged
merged 4 commits into from
Nov 5, 2018

Conversation

jeffersonrabb
Copy link
Contributor

@jeffersonrabb jeffersonrabb commented Nov 1, 2018

This adds a few pieces needed for the Map block (and similar future ones) to work following recent changes to block building and view script loading in #10405 and Automattic/wp-calypso#28066.

Fixes

  1. The Map block doesn't correspond to a pre-Gutenberg Jetpack module, so it needs a new structure for PHP registration. For normal modules this is done in their dedicated directory in modules/. Here, this is done in modules/blocks.php.
  2. The Map block has a few view-side dependencies, and this PR creates a way to declare these.

Testing instructions:

To test the Map block, it will need to be built from the https://github.com/Automattic/wp-calypso/tree/try/map-key branch. The Calypso branch resolves a naming consistency problem (map vs. maps-block) and temporarily adds a hardcoded Google Maps API key so it can be used without the changes in https://github.com/Automattic/wp-calypso/tree/try/map-key. This Calypso branch is meant only for testing with this PR, and will be deleted after this PR merges.

To test with Jetpack docker:

  1. Checkout wp-calypso branch try/map-key.
  2. Checkout jetpack branch try/block-dependencies.
  3. Build Map block by running this SDK command from the root of wp-calypso repo: npm run sdk gutenberg client/gutenberg/extensions/presets/jetpack -- --output-dir=PATH_TO_JETPACK_REPO/_inc/blocks. (Replace PATH_TO_JETPACK_REPO with relevant path).
  4. In Jetpack repo, verify that _inc/blocks/map/ exists and contains view.css|view.js|view.rtl.css.
  5. Create a post, add Map block, publish.

To test with Jurassic Ninja use this link:

https://jurassic.ninja/create/?gutenberg&gutenpack&shortlived&jetpack-beta&calypsobranch=try/map-key&branch=try/block-dependencies

Flows to verify

1: Verify that Map block loads and renders in editor and view. If the view.js or any of the dependencies are missing the block will be completely blank.

2: In view, watch Network tab to verify that all dependent Javascript files are loading. Among others you should see:

  • /wp-content/plugins/jetpack/_inc/blocks/map/view.js
  • /wp-content/plugins/gutenberg/vendor/react-dom.min
  • /wp-content/plugins/gutenberg/vendor/react.min.ab6b06d4.js (or similar)
  • /wp-content/plugins/gutenberg/build/element/index.js
  • /wp-content/plugins/gutenberg/build/i18n/index.js
  • /wp-content/plugins/gutenberg/build/api-fetch/index.js

3: Remove the map block (or create a second post without it), publish and view. In the Network tab none of the above files should be loading.

Proposed changelog entry for your changes:

  • Add module/blocks.php file for registration of blocks without corresponding modules.
  • Allow individual blocks to declare their view dependencies.

…ules. Structure for declaring block dependencies.
@jeffersonrabb jeffersonrabb requested a review from a team as a code owner November 1, 2018 12:25
@jetpackbot
Copy link

jetpackbot commented Nov 1, 2018

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 4, 2018.
Scheduled code freeze: November 27, 2018

Generated by 🚫 dangerJS

@jeffersonrabb jeffersonrabb added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Nov 1, 2018
class.jetpack-gutenberg.php Outdated Show resolved Hide resolved
@enejb
Copy link
Member

enejb commented Nov 1, 2018

Beside the minor renaming of things I think this works as expected. I tested this with the JN link above and it worked as expected.

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved
@jeherve jeherve added this to the 6.8 milestone Nov 2, 2018
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

A few minor improvements that I think we could make.

modules/blocks.php Show resolved Hide resolved
modules/blocks.php Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 2, 2018
@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 5, 2018
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It works well on my end. 👍

@kraftbj kraftbj added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 5, 2018
@kraftbj
Copy link
Contributor

kraftbj commented Nov 5, 2018

Good for me too. @jeffersonrabb feel free to merge if you're ready to have it land.

@jeffersonrabb jeffersonrabb merged commit 9d10130 into master Nov 5, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 5, 2018
@kraftbj kraftbj deleted the try/block-dependencies branch November 5, 2018 16:13
@jeherve jeherve added the Blocks label Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Map [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants