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

Allow loading block-styles on render (opt-in) + inlining #1236

Closed
wants to merge 35 commits into from

Conversation

aristath
Copy link
Member

@aristath aristath commented May 7, 2021

This PR backports all changes related to block styles loading from Gutenberg to core.

Trac ticket: https://core.trac.wordpress.org/ticket/50328


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@aristath
Copy link
Member Author

aristath commented May 7, 2021

This PR does the following:

tools/webpack/packages.js Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Outdated Show resolved Hide resolved
@@ -302,6 +328,7 @@ module.exports = function( env = { environment: 'production', watch: false, buil
...cssCopies,
...phpCopies,
...blockMetadataCopies,
...blockStylesheetCopies,
Copy link
Member

Choose a reason for hiding this comment

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

This adds 260 CSS files to subfolders in src/wp-includes/blocks. It won't scale well because they can change not only when the content of files updates but also when we modify webpack configuration. In this folder, we also have block.json files and PHP files copied over from WordPress packages with webpack. The only file that is custom is inside index.php. Ideally, we should remove all files inside src/wp-include/blocks from tracking in svn and leave it to the build process. I think it was proposed by @peterwilsoncc in https://core.trac.wordpress.org/ticket/49635.

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Outdated Show resolved Hide resolved
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

In my opinion, it is nearly ready to land. There are only some minor things left from my perspective. The configuration for SVN to ignore all CSS files that webpack builds being the biggest concern.

@aristath
Copy link
Member Author

Added the copied CSS files to the .gitignore list.

@gziolo
Copy link
Member

gziolo commented May 10, 2021

I performed a few tests with a demo post using all combinations of two filters that @aristath recommended:

add_filter( 'separate_core_block_assets', '__return_true' );
add_filter( 'styles_inline_size_limit', '__return_zero' );

I wanted to add that most of the functionality was extensively tested in Gutenberg plugin by Ari and me a few weeks/months ago. The functionality also got some testing in the plugin and was iterated upon.

So far everything works as expected in my testing. I also plan to verify with a custom block that defines CSS in block.json files that gets registered on the server.

.gitignore Outdated Show resolved Hide resolved
@gziolo
Copy link
Member

gziolo commented May 11, 2021

Done with https://core.trac.wordpress.org/changeset/50836 🎉

Amazing work @aristath 👏🏻 👏🏻 👏🏻 👏🏻 👏🏻

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.

2 participants