-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Allow developers to opt out of !important CSS rules generated from theme.json #37590
Comments
|
I think that the right place is to have a field in the theme.json itself instead of adding a filter. |
I think it's fine in theme.json, but I don't know how to propose this change (code) :) |
Just thinking : a field in theme.json can't make possible to disable !important from child theme or plugin. |
Yes, not yet but a theme.json is a better place for users, not only for developers. |
Hi Marie, I'd be interested in learning what issues does this causes for you so we can provide help. I also want to share the rationale for why we use this technique: we need to respect user choices at all times. There're blocks whose specificity is higher than 010, hence the presets classes attached by the user won't be applied if we don't use |
@jonschr, @webmandesign, @cbirdsong would any of you have a public theme in a repo, or be able to set one up, that demonstrates the issues around this - a classic theme, and a theme.json one if possible/applicable - and some reproduction steps for how to add content to a site running the theme to replicate the issues if needed. While exploring solutions it would be helpful to have some real-world scenarios to test against. |
You can test with my Michelle theme (https://wordpress.org/themes/michelle/):
Steps to reproduce the issue:
Results: I can only speak for myself. The issue was relatively fixed in WordPress 5.9.1 - meaning the font size is OK on large screens. But the real issue for me persist: WordPress still applies The theme is set with responsive typography and you can still see the issue when resizing the screen (with Michelle 1.2.0 and WordPress 5.9.1+). The only way around this is to remove In my opinion, though, there should be no If Finally, I think we should also make more distinction in "classic themes" terminology. Among "classic themes" there are themes that don't support Gutenberg editor and the ones that do. So, I suggest using this terminology:
This whole issue seems to be affecting block editor themes only. |
I can set up a sample theme later this week, but for now I can describe some issues. For font sizes, responsive typography is a big one. Even in theme.json themes you can only accomplish it through a For block editor themes, another issue is that you can only supply pixel units for fonts in However…
This is untrue! There are plenty of reasons you might want to override a value, even within a theme.json theme. For instance, this is a common pattern I use when declaring any value with .has-large-font-size {
font-size: 1.25rem;
font-size: clamp(1.25rem, 1.25rem + 0.5vw, 1.8rem);
}
This only gets into issues created for font sizing. This approach creates many similar problems in other areas (most notably colors), which like this leads me to agree here:
It should absolutely not be applied retroactively to pre-5.9/theme.json v2 themes, and I'd strongly suggest it not be applied at all, no matter how much easier it makes the job of the editor authors. |
Indeed, I experience the same issue here, but this was true also pre-WP5.9 unfortunately. I approach it similarly as you do. I kind of made peace that
Sorry, I don't have enough experience with However, with /* From this: */
.has-large-font-size {
font-size: 1.25rem;
font-size: clamp(1.25rem, 1.25rem + 0.5vw, 1.8rem);
}
/* To this: */
body {
--wp--preset--font-size--large: clamp(1.25rem, 1.25rem + 0.5vw, 1.8rem);
/**
* You should be able to use @support now for backwards compatibility,
* or simply revert to @media query approach or do whatever you like with
* the CSS variable.
* This should work with core's `!important` without any issue I think.
*/
} By setting up the CSS variable I think you can do the whole responsive typography spiel to your liking and you don't even need to declare styles for |
@glendaviesnz The remaining issues for me are, like others, around responsive styles. I use a sass mixin for smooth scaling up and down font sizes between a desktop size and a mobile size, and the !important declaration overrides it, resulting in a normal experience on desktop and incorrectly-sized text on mobile for any text that's set to small, large, xlarge, etc. For me, however, this is the same whether I'm in an add_theme_supports theme or a theme.json theme. The !important declaration slightly exacerbates this. I'm not a huge fan of using !important declarations in general, but in this particular case I don't actually have a huge problem with its use, as the user is setting a size in the Gutenberg UI and !Important helps to enforce that choice, overriding defaults that in some cases probably do need to be overridden. My biggest thing would probably be that theme developers don't really have a recommended way to add support for different sizing on mobile that would inherit the !important declaration (I see some suggestions up above that I haven't tried, but haven't seen similar stuff in the documentation). |
This is true, but that doesn't set a progressively enhanced fallback for browsers that don't support custom properties. More importantly, it doesn't fix all the existing themes that don't have anyone actively developing them and assumed no one would be coming through and adding "!important" to these classes.
Actually, these are For reference, when starting with this: add_theme_support( 'disable-custom-font-sizes' );
add_theme_support( 'editor-font-sizes', array(
array(
'name' => __( 'Small', 'block-editor-theme' ),
'size' => 12,
'slug' => 'small'
),
array(
'name' => __( 'Medium', 'block-editor-theme' ),
'size' => 16,
'slug' => 'medium'
),
array(
'name' => __( 'Large', 'block-editor-theme' ),
'size' => 24,
'slug' => 'large'
),
) ); The CSS generated looks like this:body {
--wp--preset--font-size--small: 12px;
--wp--preset--font-size--medium: 16px;
--wp--preset--font-size--large: 24px;
}
.has-small-font-size {
font-size: var(--wp--preset--font-size--small) !important;
}
.has-medium-font-size {
font-size: var(--wp--preset--font-size--medium) !important;
}
.has-large-font-size {
font-size: var(--wp--preset--font-size--large) !important;
} |
Thanks for the extra details and sample themes, everyone. I am just trying to get a couple of other unrelated PRs finished off and will hopefully have a closer look at this tomorrow. |
Update: Thanks for all the extra detail around how to replicate the specific issues caused by this change everyone. I am coming fresh to this from both the theme perspective, and the editor change side, so I have just been taking a bit of time to make sure I understand the issues correctly from both ends. I have been able to replicate the issue caused by the I am now just pondering potential solutions, and will hopefully come back with some ideas, and probably more questions also, tomorrow.
@oandregal you mention the above here, was any work ever started on the hooks for overriding the behaviour? There is this PR, but it looks like it was abandoned. |
@webmandesign it was actually difficult to find a clear definition of what constitutes a In terms of the block editor, there are really only two types of themes, Also, a
I did some testing across classic themes without any editor supports, classic theme with editor supports, classic theme with a theme.json and a block theme and they were all affected by this issue given the right combination of CSS selectors in the theme. What sort of a solution we come up with to the |
So in order to move this issue forward, I have put together a very draft PR with one solution, which takes into account the preference stated a few times that this change shouldn't be automatically applied to pre5.9/non-theme.json themes. This is just one possible approach, and there may be some This draft PR:
Pros
Cons
/cc: @oandregal |
This would work for my needs, but I'd still argue using
For instance, the example given in #29533 (comment) as a reason for using .wp-block-post-title {
color: green;
}
.wp-block-post-title h1 {
color: inherit;
}
.has-red-color {
color: red;
} (Live Codepen demo: https://codepen.io/cbirdsong/pen/gOombYv) Keeping
|
@cbirdsong this was discussed in some additional recent comments over on the PR - linking here for reference - but it was discussed there that it would be good to see this suggested fix as a temporary quick fix to at least unblock the likes of the responsive font issues while the full removal of |
It's really frustrating that this issue doesn't have a maintainable hot-fix yet beyond just dequeuing the entire preset style sheet. I was a huge fan of Gutenberg for almost 2+ years even through all the weird oddities/bugs. The 5.8+ update and the FSE release has made Gutenberg development unbearably difficult. I tried FSE and realized it's a complete paradigm shift, cancelled that adventure. I get the reasoning for a lot of the blackboxing going on with Gutenberg and the new block themes but this should only be this rigid on Block themes. Classic theme developers are freaking out left and right with all of these changes and lack of consideration for us. I solved the majority of issues that I had with the current release (inline layout/blockGap styles, wide/full align that just works, odd image block issues, integrating theme.json pretty intimately with my theme without really ever having to touch the json file itself etc...). All of this while feeling pretty confident that long-term my code shouldn't explode with future gutenberg updates. This issue though. It's unsolvable without being a maintenance nightmare. To preface, I just want hover styles on my buttons. Things I've Tried:
At the end of the day I really only need to hook a couple lines of code in the I'm sorry in advance for the ranty nature of this comment but I'm at my witts end with Gutenberg. I just want basic hover/active effects on core buttons without completely ripping apart the ecosystem or overloading the styles statically in my own theme files. I did my due diligence before going on this soapbox. I am not the first person to be talking about this. I've seen similar issues dating back almost 2+ years at this point and I haven't found one solution that seems maintainable long-term. |
@Jeremy-Bolella there is work being done for better support of hover/focus/active states for blocks. A PR was merged to allow adding of these to theme.json, and also some work on the supporting UI . |
That's great and all but neither of those solutions fully address this situation. This is a bare minimum basic page builder functionality and the fact that Gutenberg has been around for 10+ major releases without something so basic and critical to any web development project is shocking. My solution to my big rant above without causing myself a huge heartache/maintenance explosion in the future resulted in having to parse the css presets manually and tag my needed styles. It's just beyond frustrating that even page builders have more flexibility than what Gutenberg currently offers. I understood a lot of the decisions being made with all of my other issues I ran into and solved them. This issue though is taking a ludicrous amount of code just so I can hook in hover styling in a dynamic way. It's disappointing but I'm really hoping this is the last major issue I run into trying to adapt my theme to run fully on Gutenberg. At this point if I run into another quark that completely dead stops me development wise I'm dropping Gutenberg. I loved this system it made the whole contentfill process so much more seamless but it's not worth the frustration and loss of sanity just so I can give clients configurable blocks... |
@Jeremy-Bolella, it would be great, if you have the time, to outline exactly what you see is needed to fix the problems you are experiencing with this, including details about the workarounds you have tried that haven't worked, over on the related issue. |
Absolutely once I get this wrapped up I'll jump over there with what my problem is and my fix. I'm hoping when the style engine is released this fix can just be rolled into that pretty seamlessly. Seems like the css parser route I'm taking to solve the hover/active issue is close to the idea around the style engine being developed. |
Hello! Just a note to say that I'm not sure the style engine alone will be the whole solution - I think the work going on with pseudo selectors mentioned above will determine how we support active/hover styles on elements such as the button. So that's the area to watch for now. Once that logic and design is in place, the role of the style engine (at least my understanding of it) will be to parse the style rules coming from Gutenberg and output the CSS. 🍺 |
Applying !important as a blanket statement is an awful practice. By doing so you've increased specificity to the most extreme. When everything is important, nothing is important. After all, we have css @layer in all modern browsers today. Why is that not being utilized? |
We should remove all |
@djmtype
|
@cbirdsong What about @WraithKenny's idea, using the |
They do that a lot and I'm not a fan, though my reasons are getting less relevant as time passes. It is definitely a way to avoid |
The biggest issue was that there was never a reasonable excuse to use |
Hi @oandregal, considering the recent changes with the gradual reduction of CSS specificity, is it still necessary to use This still creates a lot of problems. Here is one example: Currently it is like this:
But when I try to add additional properties, they don't work:
For example, couldn't the current state be replaced with this?
Or this?
In this case, additional properties will work.
Any ideas? |
In that case, it could also be more specific and use |
I am becoming increasingly concerned that there is an attitude of "everything should be done in the editor" as Gutenberg continues to grow. I have seen comments from contributors that make it very clear that consideration of theme CSS is not a priority during development. And as Gutenberg continues to offer more controls for various styling features, so does its CSS footprint. Each update introduces new conflicts that developers have to fight, sometimes very unnecessarily (see #60922). Don't get me wrong, I absolutely love that Gutenberg creates an easy interface for creating complex content, but it can't be everything to everybody. There has to be room for theme customization. I know a lot of us have belabored this point, but it's a huge ongoing issue. |
What problem does this address?
Currently, CSS rules generated from theme.json settings get !important automatically added.
It's fine for theme authors who need to ensure that their CSS rules are not overridden by plugins or so.
But as a theme developer, working at web agency, I don't need those !important. I control the scope of custom plugins.
What is your proposed solution?
My solution is to add a filter, to opt out of adding those !important to CSS, in the
compute_preset_classes
function.eg :
add_filter( 'compute_preset_classes_important', '__return_false' );
The text was updated successfully, but these errors were encountered: