-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
TwentyTwenty: make presets back compatible with older versions #2154
TwentyTwenty: make presets back compatible with older versions #2154
Conversation
The CSS Custom Properties are only part of WordPress 5.9. To keep backward-compatibility with older versions we can't remove the regular css properties.
@@ -206,20 +206,24 @@ Inter variable font. Usage: | |||
/* GENERAL COLORS */ | |||
|
|||
.has-black-background-color { | |||
--wp--preset--color--black: #000; |
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.
Apparently, in addition to bringing back the regular CSS Properties, #2154 missed generating the CSS Custom Properties for these values. This PR also fixes it.
} | ||
|
||
.editor-styles-wrapper p.has-large-font-size { | ||
--wp--preset--font-size--large: 1.25em; | ||
font-size: 1.25em; | ||
} | ||
|
||
.editor-styles-wrapper p.has-larger-font-size { |
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.
What about the "larger" font size? Should it receive x-large
or similar preset?
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 existing issue is with the theme presets that have the same slug
than one defined by WordPress. Because WordPress doesn't define any larger
(see) this still works as before.
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.
Could it not use the x-large
slug in this case? Or does there need to be a direction naming match been, i.e. the slug would need to be larger
which isn't defined?
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.
This PR doesn't aim to make any design change, just adapting existing presets, so that change is not required for the to work. If people think that's something worth doing I'd address it as a separate issue where design is involved, I'm not the best person to respond to that question :)
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.
Isn't x-large
slug an existing preset as defined here in theme.json? If yes, couldn't that slug be used for the "larger" styles:
.editor-styles-wrapper p.has-larger-font-size {
--wp--preset--font-size--large: 1.5em;
font-size: 1.5em;
}
(Sorry if my question is redundant. Seeking to understand)
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, x-large
is a new preset introduced by WordPress 5.9 and it'll generate the following class: .has-x-large-font-size
. My point is that this PR only aims to fix how presets are updated by using CSS Custom Properties. So, as long as a theme doesn't have a .has-x-large-font-size
class it doesn't need any change.
A different matter is whether the existing .has-larger-font-size
provided by this theme can be .has-x-large-font-size
now that WordPress has introduced the x-large
preset. That's a design decision I don't have enough context about and that's not necessary for this PR. Does this help?
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.
Aha, I see. There is a direct correlation then between the slug and the CSS class generated. Cool. Thanks for your patience in explaining that to me.
In testing on WP 5.6.7 to 5.8.3, the fonts do not render the same in the block editor and front-end. The front-end is correctly using the preset font size. The block editor though is using the That said, the problem existed before this PR as the Test Env:
|
Tested with 5.9 current branch and RC2. The fonts are rendering the same in both the block editor and front-end. In the block editor, the preset does take precedence over the font-size inline on the element. Why? .editor-styles-wrapper .has-small-font-size {
font-size: var(--wp--preset--font-size--small) !important;
} |
Testing Color Presets5.9Front-end and block editor render the same. Works as expected ✅ 5.6.7, 5.7.5, 5.8.3Front-end and block editor render the same ✅ For :root .editor-styles-wrapper .has-black-color {
color: #000;
}
:root .editor-styles-wrapper .has-white-background-color {
background-color: #fff;
}
:root .has-black-color {
color: #000;
}
:root .has-white-background-color {
background-color: #fff;
}
.has-black-color {
--wp--preset--color--black: #000;
}
.has-white-background-color {
--wp--preset--color--white: #fff;
color: #000;
} For :root .editor-styles-wrapper .has-white-color {
color: #fff;
}
:root .editor-styles-wrapper .has-black-background-color {
background-color: #000;
}
:root .has-black-background-color {
background-color: #000;
}
.has-white-color {
--wp--preset--color--white: #fff;
}
.has-black-background-color {
--wp--preset--color--black: #000;
color: #fff;
} |
The presets and fallbacks are working with the exception of:
@oandregal and @desrosj What do you think? |
This is by design. The inline styles in the editor only exist to cover themes that don't add the CSS of their preset in the editor. In this case, what happens is that the CSS class of the preset has higher priority, so it's fine |
Is okay then that this theme's font-size (whether by preset or fallback) does not take affect in the block editor, meaning that the rendering is different in the block editor than in the front-end? See it in action here #2154 (comment) |
Yeah, it works as before. Nothing in this PR or the previous changed how that works. |
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.
@oandregal has answered my questions:
larger
preset does not exist; hence why the "larger" styles do not receive a preset ✅- font size rendering in the block editor on < 5.9 remains unchanged by this PR ✅
In my testing from 5.6 to 5.9, the changes in this PR work as expected (test results shared in the PR).
LGTM 👍 Thanks @oandregal!
Hey @desrosj, I think this PR resolves the issues you raised. I tested back to WP 5.6.7. Any other concerns or testing needed? |
I'm going to close this in favor of #2233 |
Trac ticket https://core.trac.wordpress.org/ticket/54782
Follow-up to #2130
WordPress 5.9 uses CSS Custom Properties to define the presets, and themes without a theme.json need to use them to set their value. However, the default themes need to work with older versions of WordPress that don't have the CSS Custom Properties, therefore we can't remove the existing regular CSS properties. This PR brings them back.
How to test
Test this both in WordPress 5.9 and WordPress 5.7. In 5.9 the CSS Custom Properties are present for all themes and in 5.7 are not. This covers all the cases.
Font sizes
small
to the 1st paragraph,normal
to the 2nd, nothing to the third,large
to the 4th, andlarger
to the 5th.The expected result is that the font size for each paragraph has the assigned value, both in the editor and front-end.
Colors
has-white-background-color has-black-color
into the "Additional classes" text field of the "Advanced" section of the block sidebar.has-black-background-color has-white-color
into the "Additional classes" text field of the "Advanced" section of the block sidebar.Via the browser devtools make sure that the first paragraph's color is
rgb(0,0,0)
and its background isrgb(255,255,255)
. The second paragraph should be the opposite.