-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
base: trunk
Are you sure you want to change the base?
Add stylesheet minification for bundled themes (Block themes) #10081
Conversation
Co-authored-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
…wenty-One themes Co-authored-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
…y Twenty, and Twenty Twenty-One theme stylesheet header comment Co-authored-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
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_DEBUG
is defined, it will check the version to see if it contains-src
as 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.css
but 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 dev
is 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 😅.
Co-authored-by: Weston Ruter <westonruter@gmail.com>
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
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-admin/css/colors/*/*.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.
None of the other default themes are included. Is there a reason to only include these two?
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, in this PR only TT2 and TT5 have their stylesheets minified, other themes with a build step do not minify their stylesheets.
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 idea is to start out with just minding the stylesheets for block themes since they are small and eligible to be inlined. Once we have a good workflow in place for block themes, we can follow up with minification for classic themes. However the performance gains won't be as large because the larger stylesheets wouldn't be eligible for inlining. A reduced size for an external stylesheet doesn't translate into as much of a performance boost as being able to eliminate that render blocking stylesheet in the first place, as I understand.
/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-theme
job before theUpload theme ZIP as an artifact
step 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
/* | ||
* IMPORTANT: This file is not normally served directly on the frontend, unless SCRIPT_DEBUG is enabled. For normal page | ||
* loads, the `style.min.css` file will be served instead of `style.css` for improved performance. Because of this, if | ||
* you make any changes to this file in the Theme File Editor, you will not normally see those changes reflected on the | ||
* frontend. You would need to navigate to the theme directory in a terminal and run `npm install && npm run build`. | ||
* Because of this, it is not recommended that you use the Theme File Editor to modify this stylesheet. Instead, you | ||
* should add the necessary style overrides via "Additional CSS" in the Site Editor. If neither of these are an option, | ||
* then you can modify `twentytwentytwo_styles()` in `functions.php` so that it never uses the minified file. | ||
*/ |
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.
/* | |
* IMPORTANT: This file is not normally served directly on the frontend, unless SCRIPT_DEBUG is enabled. For normal page | |
* loads, the `style.min.css` file will be served instead of `style.css` for improved performance. Because of this, if | |
* you make any changes to this file in the Theme File Editor, you will not normally see those changes reflected on the | |
* frontend. You would need to navigate to the theme directory in a terminal and run `npm install && npm run build`. | |
* Because of this, it is not recommended that you use the Theme File Editor to modify this stylesheet. Instead, you | |
* should add the necessary style overrides via "Additional CSS" in the Site Editor. If neither of these are an option, | |
* then you can modify `twentytwentytwo_styles()` in `functions.php` so that it never uses the minified file. | |
*/ | |
/* | |
* IMPORTANT: This file is only served on the frontend when `SCRIPT_DEBUG` is enabled; | |
* in most instances, the `style.min.css` file will be served. It is not recommended that you | |
* use the Theme File Editor to modify this stylesheet. Instead, add the necessary style | |
* overrides via "Additional CSS" in the Site Editor. | |
*/ |
There is no specific language standard for CSS, but for PHP the core standard are
Summaries should be clear, simple, and brief. Avoid describing “why” an element exists, rather, focus on documenting “what” and “when” it does something.
This aims to simplify and make the instructions a bit clearer. Further, build instructions belong in a readme file. Additionally, modifying a theme directly should be discouraged at every opportunity.
Starting this discussion here, but I think it applies to both style.css files
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.
Done in 8ceb867.
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 jorbin 😄
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.
Further, build instructions belong in a readme file.
I've added contributing.txt
files to the two themes in a9e839c, adapted from twentynineteen
.
…y-Five themes Co-authored-by: Jon Surrell <jonsurrell@git.wordpress.org>
Co-authored-by: Aaron Jorbin <jorbin@git.wordpress.org>
…ytwentyfive Co-authored-by: Aditya Dhade <aditya.dhade@rtcamp.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.