-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add stylesheet minification for bundled themes (Block themes) #10081
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
Add stylesheet minification for bundled themes (Block themes) #10081
Conversation
Co-authored-by: Shyamsundar Gadde <[email protected]>
…wenty-One themes Co-authored-by: Shyamsundar Gadde <[email protected]>
…y Twenty, and Twenty Twenty-One theme stylesheet header comment Co-authored-by: Shyamsundar Gadde <[email protected]>
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
cc: @westonruter |
|
@b1ink0 To make this easier to review and initially land, how about this initial PR be limited to just the block themes. To that end, could you revert the changes to all themes other than Twenty Twenty-Two and Twenty Twenty-Five? Once this is merged, we can then follow up with restoring the changes you currently have. Once we establish the pattern with the block themes, then it will be easier to then follow up with classic themes. The highest priority is for the block themes well, since these are the ones which will be eligible to have their theme styles inlined, since they are very small. The classic themes will likely not be eligible for inlining, so the impact of minification will be less. |
| $src = 'style.min.css'; | ||
| if ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG || strpos( wp_get_wp_version(), '-src' ) || ! file_exists( get_parent_theme_file_path( 'style.min.css' ) ) ) { |
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.
In default-constants.php, the SCRIPT_DEBUG constant is always defined. So we don't need to check for it. This can also use str_contains() instead, since the polyfill was introduced in WP 5.9.
| $src = 'style.min.css'; | |
| if ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG || strpos( wp_get_wp_version(), '-src' ) || ! file_exists( get_parent_theme_file_path( 'style.min.css' ) ) ) { | |
| if ( SCRIPT_DEBUG || str_contains( wp_get_wp_version(), '-src' ) || ! file_exists( get_parent_theme_file_path( 'style.min.css' ) ) ) { |
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.
Note that when SCRIPT_DEBUG is defined, it will check the version to see if it contains -src as well.
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.
When considering what core is doing for the emoji loader:
file_get_contents( ABSPATH . WPINC . '/js/wp-emoji-loader' . wp_scripts_get_suffix() . '.js' )I think we can skip the file_exists() check as well.
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.
If this were using get_stylesheet_uri() then we'd need the file_exists() check since it could be that the child theme has a style.css but not a style.min.css. But since we're only looking at the parent theme, then that isn't needed.
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.
See #10081 (comment) for a suggestion to tie this together.
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.
Note that when
SCRIPT_DEBUGis defined, it will check the version to see if it contains-srcas well.
Okay, default-constants.php is handling this, so the check can be removed without an issue.
If this were using
get_stylesheet_uri()then we'd need thefile_exists()check since it could be that the child theme has astyle.cssbut not astyle.min.css. But since we're only looking at the parent theme, then that isn't needed.
I have a doubt about this case. Currently, since we are not shipping minified files in the repo, the contributor needs to navigate to the theme and run npm run build to generate the minified files.
Let's say a core contributor is using the TT5 or TT2 theme and sets SCRIPT_DEBUG to false while debugging some other core scripts TT5 will then try to load the minified file, which doesn’t exist.
There are two solutions I can think of:
- Keep the file existence check.
- Create a new Webpack config that generates the minified files when
npm run devis run at the root level. (This is required because the Gruntfile only generates the minified files in the build folder.)
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 believe this is to be expected. For example, if you force SCRIPT_DEBUG to be false in core without having done a build, you get all kinds of errors related to missing scripts and styles.
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.
My bad, after setting SCRIPT_DEBUG to false and running npm run dev, it does generate the minified files for the theme in the src directory 😅.
…n, Twenty Twenty, and Twenty Twenty-One theme stylesheet header comment" This reverts commit 96cc018.
…Twenty Twenty-One themes" This reverts commit d3beb63.
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
This comment was marked as spam.
This comment was marked as spam.
|
Can this be marked as ready for review as opposed to being a draft? It would be good to get more contributors who work on bundled themes to also look. It's looking good to me!
What is this about? |
Need to work on the deployment workflow of themes with a build step. There is already logic for handling T19, T20, and TT1, but need to add support for TT2 and TT5. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @brotherk86k-lab. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…into feature/add-stylesheet-minification-for-bundled-themes
westonruter
left a comment
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.
When I turn off SCRIPT_DEBUG with Twenty Twenty-Five:
-<link rel='stylesheet' id='twentytwentyfive-style-css' href='http://localhost:8000/wp-content/themes/twentytwentyfive/style.css?ver=1.3' media='all' />
+<link rel='stylesheet' id='twentytwentyfive-style-css' href='http://localhost:8000/wp-content/themes/twentytwentyfive/style.min.css?ver=1.3' media='all' />And in Twenty Twenty-Two:
-<link rel='stylesheet' id='twentytwentytwo-style-css' href='http://localhost:8000/wp-content/themes/twentytwentytwo/style.css?ver=2.0' media='all' />
+<link rel='stylesheet' id='twentytwentytwo-style-css' href='http://localhost:8000/wp-content/themes/twentytwentytwo/style.min.css?ver=2.0' media='all' />| /src/wp-content/themes/twentytwentytwo/node_modules | ||
| /src/wp-content/themes/twentytwentyfive/node_modules | ||
|
|
||
| # Minified files in bundled themes |
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 this will have the effect of never shipping the minified files since the files won't be in the checked out code.
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.
Yes, since the minified files are ignored, the ZIP files are being prepared from the repository without the minified files. See below:
wordpress-develop/.github/workflows/test-and-zip-default-themes.yml
Lines 216 to 230 in 73e822f
| steps: | |
| - name: Checkout repository | |
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | |
| with: | |
| ref: ${{ github.event_name == 'workflow_dispatch' && inputs.branch || github.ref }} | |
| show-progress: ${{ runner.debug == '1' && 'true' || 'false' }} | |
| persist-credentials: false | |
| - name: Upload theme ZIP as an artifact | |
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 | |
| with: | |
| name: ${{ matrix.theme }} | |
| path: src/wp-content/themes/${{ matrix.theme }} | |
| if-no-files-found: error | |
| include-hidden-files: true |
There are two options I can think of to resolve this:
-
We can include the minified files in the repo, similar to how classic themes require contributors to include the built CSS and CSS map files generated from SCSS files.
-
We could add a build step for TT2 and TT5 in the workflow for the
bundle-themejob before theUpload theme ZIP as an artifactstep is run.
This would work if the release of the bundled theme is handled through a GitHub workflow.
ref: https://make.wordpress.org/core/handbook/about/release-cycle/update-bundled-themes-2/#creating-theme-zip-files
cc: @westonruter
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.
It sounds like the second option would be preferred, as I really don't like minified files being committed to the repo. For the classic themes using SASS it's a bit different because the changes to the .scss files at least result in changes to .css files which can get a clean diff. But for minified files, and changes just results in a lot of noise.
I will admit I don't have any experience with the theme release workflows, so I don't know specifically what is required to implement that second option.
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 drafted the new bundle-theme job steps with assistance from Gemini Code Assist: 39bac29
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.
It looks like the new job step is working correctly!
In the above action run, the generated artifacts now contain the minified files.
TT5: https://github.com/WordPress/wordpress-develop/actions/runs/18329688092/artifacts/4209767198
TT2: https://github.com/WordPress/wordpress-develop/actions/runs/18329688092/artifacts/4209767505
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.
Props to @sabernhardt for pointing out that the node_modules was getting included in the build artifacts! This should now be removed as of 99a0969.
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.
The Core handbook page for updating bundled themes will need an update.
I think the KISS approach would be to document that only zips created via the GitHub workflow should be used when updating the theme directory using a notice block so it's appropriately highlighted.
…y-Five themes Co-authored-by: Jon Surrell <[email protected]>
Co-authored-by: Aaron Jorbin <[email protected]>
…ytwentyfive Co-authored-by: Aditya Dhade <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: Stephen A. Bernhardt <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
peterwilsoncc
left a comment
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've added a few notes inline.
| /src/wp-content/themes/twentytwentytwo/node_modules | ||
| /src/wp-content/themes/twentytwentyfive/node_modules | ||
|
|
||
| # Minified files in bundled themes |
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.
The Core handbook page for updating bundled themes will need an update.
I think the KISS approach would be to document that only zips created via the GitHub workflow should be used when updating the theme directory using a notice block so it's appropriately highlighted.
…and package.json in themes Co-authored-by: Peter Wilson <[email protected]>
…into feature/add-stylesheet-minification-for-bundled-themes
|
This PR is a blocker for two other other tickets milestoned for 6.9: Core-63007 and Core-63018. So if there aren't any other key concerns, I want to commit this enhancement to get moving on those other tickets. Approvals appreciated. |
Co-authored-by: Peter Wilson <[email protected]>
peterwilsoncc
left a comment
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.
This looks good for commit.
I've marked a bunch of discussions as resolved, the one that remains open is to document zip generation in the core handbook for updating bundled themes in the theme repo.
This introduces a build process to provide minified stylesheets for the Twenty Twenty-Two and Twenty Twenty-Five themes. The built minified CSS is not committed to version control. When `SCRIPT_DEBUG` is disabled, the minified `style.min.css` file will be enqueued to improve performance, including reducing render-blocking resources as well as facilitating inlining. See #63007. * Stylesheets are minified via the new core `cssmin:themes` Grunt task, which is automatically run as part of `build:css`. * The same build process is added to the themes (via `package.json` and `package-lock.json`) with `build` and `watch` commands; the `postcss` and `cssnano` development dependencies are used to handle CSS minification. * The `functions.php` in each theme is updated to enqueue `style.min.css` unless `SCRIPT_DEBUG` is enabled, in which case `style.css` is enqueued. * A notice comment is added to `style.css` to warn against editing the stylesheet since the minified version will likely be served instead. * A new `contributing.txt` file is added to each theme to document the build process for developers. * The `test-and-zip-default-themes` GitHub workflow is updated to install npm dependencies and build the minified assets before packaging the themes. * New PHPUnit tests are added to verify version number consistency across `style.css`, `readme.txt`, `package.json`, and `package-lock.json` files in default themes. Developed in #10081. Props b1ink0, westonruter, shyamgadde, jonsurrell, sabernhardt, jorbin, peterwilsoncc, poena. Fixes #63012. git-svn-id: https://develop.svn.wordpress.org/trunk@60934 602fd350-edb4-49c9-b593-d223f7449a82
This introduces a build process to provide minified stylesheets for the Twenty Twenty-Two and Twenty Twenty-Five themes. The built minified CSS is not committed to version control. When `SCRIPT_DEBUG` is disabled, the minified `style.min.css` file will be enqueued to improve performance, including reducing render-blocking resources as well as facilitating inlining. See #63007. * Stylesheets are minified via the new core `cssmin:themes` Grunt task, which is automatically run as part of `build:css`. * The same build process is added to the themes (via `package.json` and `package-lock.json`) with `build` and `watch` commands; the `postcss` and `cssnano` development dependencies are used to handle CSS minification. * The `functions.php` in each theme is updated to enqueue `style.min.css` unless `SCRIPT_DEBUG` is enabled, in which case `style.css` is enqueued. * A notice comment is added to `style.css` to warn against editing the stylesheet since the minified version will likely be served instead. * A new `contributing.txt` file is added to each theme to document the build process for developers. * The `test-and-zip-default-themes` GitHub workflow is updated to install npm dependencies and build the minified assets before packaging the themes. * New PHPUnit tests are added to verify version number consistency across `style.css`, `readme.txt`, `package.json`, and `package-lock.json` files in default themes. Developed in WordPress/wordpress-develop#10081. Props b1ink0, westonruter, shyamgadde, jonsurrell, sabernhardt, jorbin, peterwilsoncc, poena. Fixes #63012. Built from https://develop.svn.wordpress.org/trunk@60934 git-svn-id: http://core.svn.wordpress.org/trunk@60270 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This introduces a build process to provide minified stylesheets for the Twenty Twenty-Two and Twenty Twenty-Five themes. The built minified CSS is not committed to version control. When `SCRIPT_DEBUG` is disabled, the minified `style.min.css` file will be enqueued to improve performance, including reducing render-blocking resources as well as facilitating inlining. See #63007. * Stylesheets are minified via the new core `cssmin:themes` Grunt task, which is automatically run as part of `build:css`. * The same build process is added to the themes (via `package.json` and `package-lock.json`) with `build` and `watch` commands; the `postcss` and `cssnano` development dependencies are used to handle CSS minification. * The `functions.php` in each theme is updated to enqueue `style.min.css` unless `SCRIPT_DEBUG` is enabled, in which case `style.css` is enqueued. * A notice comment is added to `style.css` to warn against editing the stylesheet since the minified version will likely be served instead. * A new `contributing.txt` file is added to each theme to document the build process for developers. * The `test-and-zip-default-themes` GitHub workflow is updated to install npm dependencies and build the minified assets before packaging the themes. * New PHPUnit tests are added to verify version number consistency across `style.css`, `readme.txt`, `package.json`, and `package-lock.json` files in default themes. Developed in WordPress/wordpress-develop#10081. Props b1ink0, westonruter, shyamgadde, jonsurrell, sabernhardt, jorbin, peterwilsoncc, poena. Fixes #63012. Built from https://develop.svn.wordpress.org/trunk@60934 git-svn-id: https://core.svn.wordpress.org/trunk@60270 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
Follow-up (#10279) which adds these notices to the Theme File Editor:
|





TODO:
Stylesheet minification for Twenty Nineteen, Twenty Twenty, and Twenty Twenty-One themes (Classic themes with existing build setup).Stylesheet minification for classic themes without existing build setup.Use Webpack instead of Gruntfile for a more streamlined workflow.Add server-side or client-side minification of stylesheets.Trac ticket: https://core.trac.wordpress.org/ticket/63012
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.