Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
b4b023f
37a95d7
e91f237
bb51c6d
4a57e9f
8dd0858
ba20066
2b9aced
792c595
08c6ea2
755ee39
f8098a2
751ac61
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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 useinit
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.