Blocks: load each block from a separate file, take 2 #11312

jeherve · 2019-02-08T18:48:39Z

Changes proposed in this Pull Request:

As we work to add several new modules, the existing module/blocks.php is updated quite often. It causes a few issues:

The file keeps getting longer and longer.
The longer the file, the longer it takes to skim through it.
Everyone working on that file ends up having to rebase frequently because we are merging changes to that file often.
Keeping the file in sync with WordPress.com can be more work if someone does not commit their matching WordPress.com diff immediately after merging a Jetpack PR.

Splitting things into different files makes things easier to maintain for everyone.

This commit switches to only loading a block file if:

Gutenberg is available on the site.
Gutenpack is enabled on the site.
the block is available in the block list (index.json)

About the chosen structure

I highlighted the new directory structure in a readme file here:
https://github.com/Automattic/jetpack/blob/742e263436dd2b29fd9d7592dd5c2452943f3273/extensions/readme.md

It was chosen so in the future, it can be used to host the blocks' source code as well. When that happens, things should look like this:

/extensions/blocks/
/extensions/blocks/map/
/extensions/blocks/map/*.php
/extensions/blocks/map/client/*.jsx

If the extension happens to not be a block, but a plugin for example, it can be placed in /extensions/plugins.

For further discussion about this, see:
#11286
pafL3P-io-p2

Testing instructions:

Go to Posts > Add New
Do you see, and can you use all blocks?
Beta blocks too?
Yay!

Proposed changelog entry for your changes:

None

matticbot · 2019-02-08T18:48:44Z

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D24147-code before merging this PR. Thank you!

jeherve · 2019-02-08T18:49:14Z

This already needs a rebase, on it soon.

jetpackbot · 2019-02-08T19:43:20Z

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: March 5, 2019.
Scheduled code freeze: February 26, 2019

Generated by 🚫 dangerJS against ba0c2c1

As we work to add several new modules, the existing module/blocks.php is updated quite often. It causes a few issues: - The file keeps getting longer and longer. - The longer the file, the longer it takes to skim through it. - Everyone working on that file ends up having to rebase frequently because we are merging changes to that file often. - Keeping the file in sync with WordPress.com can be more work if someone does not commit their matching WordPress.com diff immediately after merging a Jetpack PR. Splitting things into different files makes things easier to maintain for everyone. This commit switches to only loading a block file if: - Gutenberg is available on the site. - Gutenpack is enabled on the site. - the block is available in the block list (index.json) The directory structure was chosen so in the future, it can be used to host the blocks' source code as well. For further discussion about this, see: pafL3P-io-p2

matticbot · 2019-02-08T20:35:23Z

jeherve, Your synced wpcom patch D24147-code has been updated.

extensions/readme.md

simison

Tested on WP v5.0.3 with Gutenberg plugin v5.0 active and inactive.

Checked that all the blocks appear at block picker,
that I don't see beta blocks when beta constant is disabled,
that forms block has its children blocks,
that on frontend I get assets for tiled gallery and related posts blocks. 👍

I love how we've thought a bit ahead next steps when thinking naming conventions and yet not optimized too much for the future (that sometimes doesn't end up happening).

I'd encourage you to double check WP 4.9 without Gutenberg before merging and I've left some minor notes but feel free to ship! 🚢

Thanks a ton for doing this, you're making everyone's work so much easier with this!

extensions/readme.md

simison · 2019-02-08T21:17:21Z

class.jetpack-gutenberg.php

+	 *
+	 * @since 7.1.0
+	 */
+	public static function load_independent_blocks() {


There's something funky about term "independent" 🤔 ...that said I don't have anything else to suggest and I'm ok with this. 😄

I have an inkling we'll eventually one day make all our blocks module-independent and re-visit this by renaming it to load_blocks(). 😄

This may be my ESL at play :)

I wanted to denote that those blocks aren't linked to any module. Happy to change that of course, I just can't think of a better word right now.

We actually do have module dependent block(s) in that folder:

jetpack/extensions/blocks/tiled-gallery/tiled-gallery.php

Line 12 in 02e53f8

class_exists( 'Jetpack_Photon' ) && Jetpack::is_module_active( 'photon' )

I'd be up moving rest of the module-dependent blocks here too and do something similar, e.g. this could be good to move:

jetpack/modules/related-posts/jetpack-related-posts.php

Lines 71 to 78 in 02e53f8

if ( function_exists( 'register_block_type' ) ) {

register_block_type(

'jetpack/related-posts',

array(

'render_callback' => array( $this, 'render_block' ),

)

);

}

(related p9dueE-vJ-p2, cc @aldavigdis )

...but that's for another day and PR. :-)

You're right. It would make even more sense once we move the blocks themselves in there; it will be better and easier for everyone to have everything in one place.

Once this gets merged, I'll take a look at this.

Getting this started in #11325

See #11312 (comment) We can add this back once the move happens.

See #11312 (comment)

class.jetpack-gutenberg.php

See #11312 (comment)

matticbot · 2019-02-08T21:38:35Z

jeherve, Your synced wpcom patch D24147-code has been updated.

extensions/blocks/business-hours/business-hours.php

See #11312 (comment)

ockham

Thanks Jeremy, this is looking great!

Did some smoke testing (inserting blocks and checking them on the frontend).
Also downgraded to WP 4.9.9 to verify that there are no fatals 😅

jeffersonrabb · 2019-02-11T11:28:30Z

I took a quick look at the blocks I'm involved with (Map|Mailchimp|Slideshow|Gif) and all looks good.

brbrr

It works well in my tests. LGTM 🚢

Following the changes we've made in #11312, I think it now makes sense to bring block registration closer to where the block's code will eventually live, when possible. See #11312 (comment)

jeherve added [Status] In Progress [Type] Janitorial [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Feb 8, 2019

jeherve added this to the 7.1 milestone Feb 8, 2019

jeherve self-assigned this Feb 8, 2019

jeherve requested a review from a team as a code owner February 8, 2019 18:48

matticbot added the Touches WP.com Files label Feb 8, 2019

jeherve mentioned this pull request Feb 8, 2019

Blocks: load each block from a separate file. #11286

Closed

jeherve force-pushed the split/block-files-take-2 branch from 611b94a to 742e263 Compare February 8, 2019 19:41

jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Feb 8, 2019

jeherve requested review from a team February 8, 2019 19:58

simison requested review from a team and removed request for a team February 8, 2019 20:07

jeherve force-pushed the split/block-files-take-2 branch from 742e263 to 12fa4fd Compare February 8, 2019 20:35

simison reviewed Feb 8, 2019

View reviewed changes

extensions/readme.md Outdated Show resolved Hide resolved

simison previously approved these changes Feb 8, 2019

View reviewed changes

Remove block js details from readme.

02e53f8

See #11312 (comment) We can add this back once the move happens.

jeherve dismissed simison’s stale review via 02e53f8 February 8, 2019 21:30

jeherve force-pushed the split/block-files-take-2 branch from 77cecb1 to 02e53f8 Compare February 8, 2019 21:30

Capitalize readme file name.

72f0665

See #11312 (comment)

eliorivero reviewed Feb 8, 2019

View reviewed changes

class.jetpack-gutenberg.php Show resolved Hide resolved

Block loading: update as per feedback.

1f19d8b

See #11312 (comment)

ockham mentioned this pull request Feb 11, 2019

Gutenberg: Add and use jetpack_register_block_type() wrapper #11310

Merged

ockham reviewed Feb 11, 2019

View reviewed changes

extensions/blocks/business-hours/business-hours.php Outdated Show resolved Hide resolved

Business Hours Block: fix typo

ba0c2c1

See #11312 (comment)

ockham approved these changes Feb 11, 2019

View reviewed changes

brbrr approved these changes Feb 11, 2019

View reviewed changes

simison mentioned this pull request Feb 11, 2019

Repeat Visitor: register new block and assets #11224

Merged

jeherve merged commit 661d73f into master Feb 11, 2019

matticbot added the [Status] Needs Changelog label Feb 11, 2019

jeherve deleted the split/block-files-take-2 branch February 11, 2019 14:13

jeherve removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Changelog labels Feb 11, 2019

This was referenced Feb 11, 2019

Mailchimp Block: UI Review Updates #11317

Merged

Add AMP support for GIF block #11318

Merged

Update business hours block schema #11306

Merged

jeherve mentioned this pull request Feb 12, 2019

Markdown: move block to its own file. #11325

Merged

dereksmart mentioned this pull request Aug 30, 2019

Remove deprecated module stubs #13375

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: load each block from a separate file, take 2 #11312

Blocks: load each block from a separate file, take 2 #11312

jeherve commented Feb 8, 2019 •

edited

Loading

matticbot commented Feb 8, 2019

jeherve commented Feb 8, 2019

jetpackbot commented Feb 8, 2019 •

edited

Loading

matticbot commented Feb 8, 2019

simison left a comment

simison Feb 8, 2019 •

edited

Loading

jeherve Feb 8, 2019

simison Feb 8, 2019 •

edited

Loading

jeherve Feb 8, 2019

jeherve Feb 12, 2019

matticbot commented Feb 8, 2019

ockham left a comment

jeffersonrabb commented Feb 11, 2019

brbrr left a comment

	if ( function_exists( 'register_block_type' ) ) {
	register_block_type(
	'jetpack/related-posts',
	array(
	'render_callback' => array( $this, 'render_block' ),
	)
	);
	}

Blocks: load each block from a separate file, take 2 #11312

Blocks: load each block from a separate file, take 2 #11312

Conversation

jeherve commented Feb 8, 2019 • edited Loading

Changes proposed in this Pull Request:

About the chosen structure

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Feb 8, 2019

jeherve commented Feb 8, 2019

jetpackbot commented Feb 8, 2019 • edited Loading

matticbot commented Feb 8, 2019

simison left a comment

Choose a reason for hiding this comment

simison Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

jeherve Feb 8, 2019

Choose a reason for hiding this comment

simison Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

jeherve Feb 8, 2019

Choose a reason for hiding this comment

jeherve Feb 12, 2019

Choose a reason for hiding this comment

matticbot commented Feb 8, 2019

ockham left a comment

Choose a reason for hiding this comment

jeffersonrabb commented Feb 11, 2019

brbrr left a comment

Choose a reason for hiding this comment

jeherve commented Feb 8, 2019 •

edited

Loading

jetpackbot commented Feb 8, 2019 •

edited

Loading

simison Feb 8, 2019 •

edited

Loading

simison Feb 8, 2019 •

edited

Loading