Do not add to the block-library CSS bundle the colors that come from theme.json
#33924
Conversation
oandregal
added
[Package] Base styles
|
Size Change: -2.81 kB (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
|
The principle seems good to me. Is there anything specific you'd like feedback on or steps to test? |
|
I'm mostly interested in confirming the direction makes sense for other folks (cc @youknowriad ). I've added some testing instructions for this. I believe this is the only place where these are used but welcome double-check. |
|
Following your test steps, I can just confirm things work as expected. I can't fully wrap my head around any ill side effects, but trimming things always seems sensible to me. |
| @@ -55,13 +55,13 @@ | |||
|
|
|||
| :root .editor-styles-wrapper { | |||
| // Background colors. | |||
| @include background-colors(); | |||
There was a problem hiding this comment.
Are these mixins still necessary/used somewhere? Should we remove them as well?
There was a problem hiding this comment.
I haven't found any place where they're used. The reason I didn't remove them was that they're part of the base-styles package API so I'm not sure how do we want to go about that: do we want to keep them for third parties?
There was a problem hiding this comment.
That's a development package, so I think you can just remove them and add a breaking change note to the changelog file
There was a problem hiding this comment.
The question I have is what happens when you instantiate a default block editor instance (just JS no backend), will you get decent defaults for theme.json configs/palettes/styles... How does that work?
There was a problem hiding this comment.
My understanding is that the block-editor was never concerned with providing the CSS for default colors, the CSS was there by other means.
Historically, that CSS was provided by the block-library and in current trunk is provided by the block-library and the server code for global styles. This PR removes the block-library CSS. It doesn't change anything for the block-editor. Is this your reading as well?
There was a problem hiding this comment.
This PR removes the block-library CSS. It doesn't change anything for the block-editor. Is this your reading as well?
It seems like you're right, though it does bother me a bit because block-library is something third-party editors can easily load but the theme.json CSS is not.
Take a look at the playground for instance https://wordpress.github.io/gutenberg/?path=/story/playground-block-editor--default
You can see that the block-library stylesheets are loaded there but with this PR they won't be. This won't break styles of the editor immediately because we add the "inline styles" for colors for backward compatibility but the classNames won't have any impact.
So it seems for me, this PR should be fine to land because it doesn't have a conceptual impact but it highlights one that we should tackle at some point. What should we do with the styles generated from theme.json: are these styles that the block editor should render based on its settings? Does the block editor work as it should without them? Should this style generation be baked or at least have a dedicated API/function in the frontend.
There was a problem hiding this comment.
Ok, I'd need a 👍 to land this then.
Do we want to add a breaking change to the block-library changelog as well? It's not directly an "API" but it seems consumers would be expecting those styles anyway.
I can look at what you mention as a follow-up. I'm around that area with #32702 so can probably dig deeper.
There was a problem hiding this comment.
Created an issue for it at #34005
There was a problem hiding this comment.
Yeah, let's add the changelog mention and ship.
|
Also created #34006 to follow suit with font sizes. |
oandregal
force-pushed
the
remove/colors-enqueued-by-theme-json
branch
from
dff2e85
to
465777f
Compare
|
It just occurred to me one case in which this breaks: classic themes that don't provide a palette. Test case:
This can be solved by outputting the classes we generate in such a case as well (we already output them if the theme declares support for link color). I'll prepare a fix. |
|
@nosolosw This also has a major impact for people using For detailed posts on what I am talking about including use cases you can have a look at those 2 posts:
So while I do think that trimming styles down for themes using |
Hi Thomas 👋 These are my thoughts: this is a step towards the goal of shipping the minimal CSS possible. The color classes aren't used by the block library directly. That we shipped them via the When I created this PR, I wasn't convinced of removing the mixing from the |
|
First: I totally agree with the goal of shipping the minimal CSS possible. So I am by no means against this, I am just thinking about backwards compatibility. So, what I am currently doing (as detailed in the linked posts) is building custom versions of All this is under the assumption that in a classic theme that doesn't include the default color styles in the theme CSS those blocks using them will now go unstyled, or am I getting something wrong here? |
Absolutely yes to this. I've already shared a test case at #33924 (comment) and I'm preparing a fix. My question was if you have found additional things that broke (related to building custom blocks or any other thing) so I could look at them. |
|
Yeah, right, of course, you already mentioned that... 🤦 No, nothing else yet, but I have to admit that I haven't actually tried yet, just been thinking about it in theory since until here I've only cared about Core releases 😇 I'll try to find some time to do so. 🤞 |
|
Prepared follow-up for classic themes at #34067 |
|
I came across this and #34067 that being carried out in a subsequent PR or that not possible because of the issues raised at #33924 (comment) ? |
|
@skorasaurus I commented there #24684 (comment) |
|
Second try at #34510 |
This PR reduces the amount of CSS imported by the
block-librarypackage, to only the ones that are not defined bytheme.jsondefaults.Alternatives I've considered: remove the mixinsUpdated: they were removed as per this convo.background-colors,foreground-colors,gradient-colors. The reason I discarded this approach is that I understand it may be an API others use and we don't want to keep them in thebase-stylespackage. Happy to reconsider if others think differently.Test instructions
I'm using the
emptythemefor this testing.The following instructions work for both the front-end and the editor:
wp-block-library-cssstylesheet.very-light-graycolor:trunk, if you inspected the same stylesheet you should also find the CSS for theblackcolor:Follow-ups
block-editoras a standalone package #34005