Repository navigation
Leverage @wordpress/base-styles admin schemes stylesheet instead of duplicate declarations in various packages - #69130
Merged
Merged
Conversation
… 3rd parties and WordPress Core
…chemes` style dependency
fabiankaegy
added
[Type] Enhancement
fabiankaegy
requested review from
Mamaduka,
carolinan,
karmatosed and
t-hamano
February 10, 2025 23:06
|
Size Change: -9.37 kB (-0.36%) Total Size: 2.57 MB
ℹ️ View Unchanged
|
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
|
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 |
Member
Author
|
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 :) |
This comment was marked as resolved.
This comment was marked as resolved.
Member
Author
|
@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 |
Contributor
|
I overall agree with this approach. This stylesheet should be loaded in all WP-Admin pages. |
karmatosed
reviewed
Feb 15, 2025
karmatosed
left a comment
karmatosed
left a comment
Member
There was a problem hiding this comment.
In testing everything works as expected - thank you. I am excited to get this in.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
t-hamano
reviewed
Feb 17, 2025
This comment was marked as resolved.
This comment was marked as resolved.
fabiankaegy
marked this pull request as ready for review
February 17, 2025 13:49
fabiankaegy
requested review from
adamziel,
draganescu,
ellatrix,
kevin940726 and
spacedmonkey
as code owners
February 17, 2025 13:49
t-hamano
reviewed
Nov 26, 2025
fabiankaegy
force-pushed
the
feature/leverage-build-base-styles
branch
from
December 12, 2025 10:56
7e52484 to
9397ad2
Compare
Member
Author
|
Hey @t-hamano 👋 I've made all the updates you've suggested. I also updated the wp core patch here with the same updates: WordPress/wordpress-develop#8909 Would love to get this merged soon to unblock the Admin reskin work for 7.0 :) Let me know if you have any additional questions / concerns. |
Contributor
|
Thanks for the update! Perhaps the restriction of export fields in this PR is causing sass-loader to fail to resolve modules, causing Storybook to fail to build. This can be resolved by exporting all assets as shown below, but I'm not sure if this is the ideal approach 🤔 diff --git a/packages/base-styles/package.json b/packages/base-styles/package.json
index 967e5fc027..6806f910a3 100644
--- a/packages/base-styles/package.json
+++ b/packages/base-styles/package.json
@@ -28,7 +28,14 @@
"module": "index.js",
"exports": {
"./package.json": "./package.json",
- "./build-style/": "./build-style/"
+ "./build-style/": "./build-style/",
+ "./variables": "./_variables.scss",
+ "./colors": "./_colors.scss",
+ "./mixins": "./_mixins.scss",
+ "./breakpoints": "./_breakpoints.scss",
+ "./animations": "./_animations.scss",
+ "./z-index": "./_z-index.scss",
+ "./default-custom-properties": "./_default-custom-properties.scss"
},
"wpScript": true,
"publishConfig": {Another concern is that there are two projects related to redesigning the admin screens or design system. I'm wondering if these are separate efforts, or if there's any connection between them.
cc @WordPress/gutenberg-components |
Member
Author
|
@t-hamano from my vantage point they are two entirely separate project that have different goals. The work from the components team happening here in the Gutenberg repo in #71196 is for the eventual complete rewrite of the wp admin in react. That is the long term goal and not in scope for 7.0 / doesn't even have a clear timeline yet. It is super important and I wish we could just ship that. But it isn't ready. So because the current design of the admin has diverged so much over time to what is happening in the site editor / block editor, the CSS only reskin for 7.0 has been created as a small scrappy project to just modernize it a bit with what we already have :) |
Contributor
|
@fabiankaegy Thanks for the detailed explanation! Now let's fix the issues in the storybook build and ship this PR. |
This is needed for the storybook build for example
Member
Author
t-hamano
approved these changes
Dec 15, 2025
Member
Author
|
@t-hamano 🤦♂️I forgot to add the props. Sorry about that |
youknowriad
reviewed
Dec 15, 2025
| // Until #37466, we can't specifically add them as editor styles yet, | ||
| // so we must hard-code it here as a dependency. | ||
| 'wp-block-editor-content', | ||
| 'wp-base-styles', |
Contributor
There was a problem hiding this comment.
It feels a bit weird to me that this is added to the blocks editor styles. Logically, it seems better to add as a dependency to all the scripts that do React.render instead (top level scripts like edit-post, edit-site, ...)
Contributor
There was a problem hiding this comment.
The user color scheme must be respected even within an iframe. For example, a block may have form elements.
So we added a style to it. Is there a better way?
Contributor
There was a problem hiding this comment.
I think that's ok, but how are we adding this dependency to the "top level" packages. I think it's not a guarantee that these top packages will depend on "blocks", especially if we start lazy-loading the editor for instance in the site editor.
Anyway, not a huge deal for now as long as it works :)
Contributor
|
It probably didn't start with this PR but I noticed that the new "fonts" page (and any boot related pages) aren't reacting to the user profile properly yet. |
Contributor
This appears to be because the boot-related pages use a theme provider. The theme provider outputs similar CSS variables via legacyWpAdminThemeOverridesCSS, but these CSS variables do not reflect the user's color scheme. Perhaps we can remove the output of this legacy CSS variable from the theme provider. |
Contributor
This is a good question I don't know :P I feel like both the "ThemeProvider" and these mixins have the same purpose. I think maybe the right fix is to actually feed the "ThemeProvider" the right color based on the WP user profile (instead of a hard-coded value like today). cc @aduth what do you think? |
Contributor
Interesting, we probably need to extend the user endpoint to expose the color setting, right? |
Contributor
Seems like it yes. |
Member
|
Yes, my expectation is that how the component receives the primary seed color value would be how we can expect to support the existing user color preference, which would generate both the new and established CSS properties for colors (backwards and future compatible). |
Member
|
Exposing the color preference through the user endpoint might be a good idea generally, though I'm not sure if we strictly need it. We might be able to pass through † Passing through could be done at the consumer level or maybe |
Member
|
Following-up on user preference theming in #74011 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
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