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

Jetpack Blocks: add index.js for each block #29298

Merged
merged 14 commits into from
Dec 11, 2018

Conversation

enejb
Copy link
Member

@enejb enejb commented Dec 11, 2018

In this change we are refactoring editor.js files into tow files ediort.js and index.js.
editor.js is now responsible for just registering the block and index.js is there to define all the blocks.

This makes it easier to separate the code that defines the block from calling it. Which provides us with the flexibility in terms of when we call the register block code.

Testing instructions

Make sure all the blocks still work as expetected in calypso
Create a jetpack bundle. Does the jetpack side still work as expected?
Does the .com wp-admin side still work as expected.

@enejb enejb added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 11, 2018
@enejb enejb self-assigned this Dec 11, 2018
@enejb enejb requested a review from simison December 11, 2018 01:50
@matticbot
Copy link
Contributor

import JetpackFieldMultiple from './components/jetpack-field-multiple';
import { __ } from 'gutenberg/extensions/presets/jetpack/utils/i18n';
import renderMaterialIcon from 'gutenberg/extensions/presets/jetpack/utils/render-material-icon';
import * as form from './index';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer named imports here (import { fields, name, settings } from './index') since it makes clearer what it is that we're requiring from that file.

@@ -26,7 +26,6 @@ const WPCOM_UNSUPPORTED_CORE_BLOCKS = [
const loadA8CExtensions = () => {
// This will also load required TinyMCE plugins via Calypso's TinyMCE component
require( '../extensions/classic-block/editor' );
require( 'gutenberg/extensions/presets/jetpack/editor.js' );
Copy link
Contributor

Choose a reason for hiding this comment

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

This is obsolete, since block registration/unregistration in Calypso is handled by refreshRegistrations() in the route handler.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

VR block isn't registered on Jetpack sites, even when JETPACK_BETA_BLOCKS constant is true.

* Internal dependencies
*/
import { __, _x } from 'gutenberg/extensions/presets/jetpack/utils/i18n';
import { default as edit } from './edit';
Copy link
Member

Choose a reason for hiding this comment

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

I know this was like that before, but couldn't we just have import edit from './edit' here?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that would read better

edit: VRImageEdit,
save: VRImageSave,
} );
registerJetpackBlock( name, settings );
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should use registerBlockType() here - there is no VR module, so the VR block should always be registered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, thinking about it some more -- we should address this on the JP side, since otherwise, the VR block will always be visible (regardless of plan or other criteria). We have precedent -- there's no Map module either, so we're registering it in https://github.com/Automattic/jetpack/blob/b25ebc43d48f32d587f9a5c8b9ab4c4f96e3767a/modules/blocks.php

Copy link
Member

@tyxla tyxla Dec 11, 2018

Choose a reason for hiding this comment

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

That's fair, let's do that on the Jetpack side. Consistency wins.

Copy link
Member

Choose a reason for hiding this comment

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

Jetpack PR to register it on the server side: Automattic/jetpack#10934

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Tests great in Calypso and Jetpack. LGTM 🚢

We should only follow up in Jetpack and register the VR block on the server side.

@ockham ockham merged commit 9c65e61 into master Dec 11, 2018
@ockham ockham deleted the update/jetpack-preset-block-index branch December 11, 2018 13:07
@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 Dec 11, 2018
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.

4 participants