Leverage @wordpress/base-styles admin schemes stylesheet instead of duplicate declarations in various packages #69130
+27
−21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 3rd parties and WordPress Core
…chemes` style dependency
fabiankaegy
added
[Type] Enhancement
fabiankaegy
requested review from
Mamaduka,
karmatosed,
carolinan and
t-hamano
|
Size Change: -5.68 kB (-0.31%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a968008. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13252228815
|
|
Thanks for the PR! Are there any discussions or concrete plans for the future regarding this? I would like to understand what this PR ultimately wants to achieve. By the way, it may also be worth knowing that the |
|
Hi @t-hamano 👋 The reason behind this PR here is that @karmatosed and I are working working on some CSS refreshes to the general wp-admin css (outside of the editor). In order to make these updates one of the first goals is to replace a lot of the hardcoded duplicate color values in the core css with the appropriate CSS custom properties that are coming from this base-styles package. Today we are using the mixin in several packages and therefore have that CSS duplicated in lots of places. By moving it into a separate CSS file that gets enqueued in WordPress Core we can remove all that duplication and also make it available everywhere to be used. As far as the updating to modern dart sass syntax etc thats something that should be done yes, but it has nothing to do with this first step of simply adding a built version of the generated output to the NPM package that we can then consume in core :) Let me know if you have any additional questions or concerns :) |
|
Thanks for the details. Personally, I feel it's redundant to build a CSS file just to use CSS custom properties in the core. My understanding is that parts of the core use SCSS. We you should be able to include the diff example in wordpress-developdiff --git a/package.json b/package.json
index 30b5be5e3b..dc71687eb0 100644
--- a/package.json
+++ b/package.json
@@ -28,6 +28,7 @@
"@playwright/test": "1.49.1",
"@pmmmwh/react-refresh-webpack-plugin": "0.5.15",
"@wordpress/babel-preset-default": "8.8.2",
+ "@wordpress/base-styles": "5.17.0",
"@wordpress/dependency-extraction-webpack-plugin": "6.8.3",
"@wordpress/e2e-test-utils": "11.8.2",
"@wordpress/e2e-test-utils-playwright": "1.8.1",
diff --git a/src/wp-admin/css/colors/_admin.scss b/src/wp-admin/css/colors/_admin.scss
index 5431da789d..89c810dba2 100644
--- a/src/wp-admin/css/colors/_admin.scss
+++ b/src/wp-admin/css/colors/_admin.scss
@@ -1,6 +1,9 @@
@import 'variables';
@import 'mixins';
+@import 'node_modules/@wordpress/base-styles/mixins';
+@import "node_modules/@wordpress/base-styles/variables";
+@include wordpress-admin-schemes();
/**
* This function name uses British English to maintain backward compatibility, as developers
@@ -644,7 +647,7 @@ div#wp-responsive-toggle a:before {
}However, the core already uses dart-sass. So this simple approach will generate a lot of warnings when building. So I think a more ideal approach would require refactoring I'd also like to hear other people's advice on what the best approach would be. cc @WordPress/gutenberg-core |
|
@t-hamano the reason why I am leaning to the built file here is that we want to reduce the duplication across the various packages. So having one shared CSS file that outputs all these variables is preferred to including the Mixins in all places. And since we want that file to exist in core and in Gutenberg distributing it here this way seems the most flexible but also easiest |
|
I overall agree with this approach. This stylesheet should be loaded in all WP-Admin pages. |
karmatosed
reviewed
Feb 15, 2025
t-hamano
reviewed
Feb 17, 2025
There was a problem hiding this comment.
I think the iframe document also needs this stylesheet, otherwise the color scheme won't be applied in the iframe document:
| This PR | trunk |
|---|---|
 |
 |
I can't remember correctly, but I think there should be some process somewhere that copies the stylesheet from the root document to the iframe document.
Another concern is how to backport this PR to core. At the very least, I think the CSS file generated by #69128 should be bundled into core before shipping this PR.
| @@ -24,6 +24,7 @@ | |||
| "node": ">=18.12.0", | |||
| "npm": ">=8.19.2" | |||
| }, | |||
| "wpScript": true, | |||
There was a problem hiding this comment.
Is there any reason for adding this? If I recall correctly, this field is required for npm workspaces. See https://github.com/WordPress/gutenberg/pull/66272/files
There was a problem hiding this comment.
The reason why I added this was that it apparently gets used by the script that copies any dist files from the individual packages into the top level dist folder which is needed in order to properly enqueue this in the Gutenberg plugin.
t-hamano
reviewed
Feb 17, 2025
We will probably need to create a new Example: gutenberg/lib/compat/wordpress-6.3/script-loader.php Lines 77 to 85 in 5cc940b |
fabiankaegy
requested review from
spacedmonkey,
draganescu,
adamziel,
kevin940726 and
ellatrix
as code owners
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Warning
This PR contains the changes of #69128. It is dependent on those changes
What?
Continuation of #69128
Replace any instances of
@include wordpress-admin-schemes()in the various packages with thewp-admin-schemesstyle handle.Why?
To remove the duplication of these styles. This is a first step before we add the
wp-admin-schemeshandle on every single wp-admin page so that the CSS custom properties can get leveraged everywhere in core.How?
By adding a new style handle called
wp-admin-schemesthat we use as a dependency instead of manually including the mixin in all the packages individually.Testing Instructions