-
Notifications
You must be signed in to change notification settings - Fork 798
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
Gutenpack: Only load frontend assets as required #10405
Gutenpack: Only load frontend assets as required #10405
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: November 6, 2018. Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't seem to see any tiled gallery on the front end of my site when inserting a gallery via the Gutenberg block. I consequently do not see the gallery assets in the source code either. Maybe that's partly because #10375 has not been merged yet?
I also left a few comments about minor coding standards changes you could make here, so our code editors don't shout at us when we open the file :)
There also seems to be a small issue with how the file is built for RTL sites.
@lezama Thanks! We still have that RTL issue though: And the missing docblocks. :) |
@jeherve Can you elaborate on the RTL issue? I didn't find any issues but I also wasn't able to test it properly since the files are not there. |
Nevermind good catch on the RTL issue. 🤦♂️ |
When testing locally (JP docker) I'm getting |
Thanks for testing the PR @jeffersonrabb. I also am running into the same issue. With the tile gallery. |
The tiled gallery currently in It might make sense to update our testing block |
return (bool) apply_filters( 'jetpack_gutenberg', true ); | ||
} | ||
|
||
public static function load_assets_as_required( $type ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we could use a filter inside this function, allowing us (or a third party) to disable loading of these in the last minute. It would be useful if someone needs a more granular control over what is to be loaded on a per block basis (contrary to jetpack_gutenberg
, which applies to all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. But do we want those filters right now? I think they can be done in a different PR when they are requested. There is also the previous way of dequeuing the assets. Which is done just before enqueuing them.
*/ | ||
if ( apply_filters( 'jetpack_gutenberg_cdn', false ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to completely be removing loading of the assets from CDN option. While we don't use it much anymore, I don't see any mention about it in this PR. It would be nice to make sure we remove it properly and we don't rely on it being here anywhere - sounds like material for another PR to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No there is still the CDN option but only for the editor assets. I didn't want to remove it since it was already taken care of by a different PR.
@@ -537,7 +537,7 @@ private function __construct() { | |||
* Prepare Gutenberg Editor functionality | |||
*/ | |||
require_once JETPACK__PLUGIN_DIR . 'class.jetpack-gutenberg.php'; | |||
add_action( 'enqueue_block_assets', array( 'Jetpack_Gutenberg', 'enqueue_block_assets' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we'll no longer use core Gutenberg's hook enqueue_block_assets
to enqueue the assets. Core Gutenberg recommends using that hook, because it assures that its necessary assets will already be loaded. Do we want to use it after all, even if we use init
for our business logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are fine with not using it. since the current enqueue_block_assets hood also fires on the front end and results in assets being loaded when they don't have to be.
12ffc7a
to
4b8a0eb
Compare
Rebased and updated to account for #10405 (comment) It's hard to test with just Tiled Galleries, though. |
This Removed the loading of the front end code that comes as a bundle of all the files. And make sure that it loads only the front end code that is needed when the block is going to be displayed.
This lets us register the tiled gallery code to be loaded on the front end only
We'll rely on those functions a lot in the next few months, the different comments should help us get all the info we need in our IDEs. It also acts as a good example for other folks who may be interested in how Jetpack adds support for its blocks.
54940f3
to
f8098a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Noting that #10491 reverted some of our work here. |
Fixes #10325
Requires you to build the blocks via this branch:
Automattic/wp-calypso#28066
This PR implement a new
jetpack_register_block()
function. Which helps Jetpack have a better understand what blocks are supported and active. Different modules could be disabled. This information could then also be used in the editor to show/hide the blocks there as well.I only added the tiled-gallery block because for now it is the only block that has frontend assets.
Testing instructions:
URL: JN test
Create a new tile gallery block.
Notice that the tile gallery view.js is only loaded on the page where the block appears.
Proposed changelog entry for your changes:
Only load front end assets as required by the block.