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.

Sign up for GitHub

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

Jump to bottom

Gutenblocks: Try Module Activation State based Visibility #10243

Closed
wants to merge 1 commit into from
Closed

Gutenblocks: Try Module Activation State based Visibility #10243

wants to merge 1 commit into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Sep 27, 2018

WIP, doesn't work yet. It's late, going to debug tomorrow.

Trying to implement what we discussed on our video call earlier today -- loading block assets (and registering them) only if the corresponding module is active.

To test:

  • Remove built blocks from _inc/blocks/, if any.
  • In Calypso, build the Tiled Gallery block only (not the entire Jetpack preset)
  • Copy to _inc/blocks/tilled-gallery/.
    (Doesn't work yet)

TODO:

  • Get to work
  • Moar elegant block slug handling

@ockham ockham added [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Sep 27, 2018
@ockham ockham self-assigned this Sep 27, 2018
@ockham ockham requested a review from a team September 27, 2018 23:09
@ockham ockham requested a review from a team as a code owner September 27, 2018 23:09
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D18921-code. (newly created revision)

@jetpackbot
Copy link

Warnings
⚠️

"Testing instructions" are missing for this PR. Please add some
⚠️

"Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS

@ockham ockham force-pushed the try/dead-simple-block-visibility branch from fbbb276 to 864dd54 Compare September 27, 2018 23:12
kraftbj
kraftbj requested changes Sep 29, 2018
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

I realize the PR is not yet marked for review; just noticed the anonymous function and wanted to flag that. Would still need a formal review whenever it is ready.

modules/tiled-gallery/tiled-gallery.php
@@ -294,3 +294,6 @@ function setting_html() {
}

add_action( 'init', array( 'Jetpack_Tiled_Gallery', 'init' ) );
add_action( 'init', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Before this could be merged, it would need to be converted to a function. Anonymous functions aren't allowed in PHP 5.2.

@sirreal
Copy link
Member

sirreal commented Oct 8, 2018

I see some advantages to building a single script from the SDK, enqueuing it, and exposing the necessary data to determine which blocks to load.

  • Single bundle lets webpack do its job deduplicating and splitting which leads to smaller bundles and less redundant code to load.
  • Build and publish are simpler and follow the SDK's current assumptions and practices (build a single entry point from preset).
  • I believe this pattern will more closely match the approach when we make a decision on the data question (pafL3P-2q-p2)

@jeherve jeherve added this to the 6.7 milestone Oct 9, 2018
@brbrr brbrr added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Oct 14, 2018
@jeherve
Copy link
Member

jeherve commented Oct 15, 2018

Related: #10198

@sirreal sirreal mentioned this pull request Oct 16, 2018
Merged
@jeherve
Copy link
Member

jeherve commented Oct 22, 2018

Closing as this has now been replaced by #10327

@jeherve jeherve closed this Oct 22, 2018
@kraftbj kraftbj deleted the try/dead-simple-block-visibility branch October 24, 2018 14:54
jeherve pushed a commit that referenced this pull request Oct 30, 2018
@sirreal @jeherve
Gutenpack: Expose data for blocks (#10327)
bd3b021 
Closes #10243 (supersedes)
Part of #10198

Calypso (block) PR in progress: Automattic/wp-calypso#27875

This is the simplest way of reusing an existing data set to expose some required data for blocks to access.

#### Changes proposed in this Pull Request:

* Exposes `Jetpack_Initial_State` global. This is the same data used by the React-based dashboard/settings views

#### Testing instructions:

* Load the Gutenberg editor with Jetpack blocks available
* Observe the `Jetpack_Initial_State` global is available in the console
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj requested changes

Assignees

@ockham ockham

Labels
[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
Milestone
6.7
Development

Successfully merging this pull request may close these issues.
7 participants
@ockham @matticbot @jetpackbot @sirreal @jeherve @kraftbj @brbrr