-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Font sizes: update default values #37381
Conversation
Size Change: +319 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
@@ -6,6 +6,9 @@ | |||
@include background-colors-deprecated(); | |||
@include foreground-colors-deprecated(); | |||
@include gradient-colors-deprecated(); | |||
--wp--preset--font-size--normal: 16px; |
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.
Shouldn't these be defined in the mixin instead?
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.
Had to remove the mixin #37381 (comment)
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.
Got it. Let's add a comment that these are for compatibility as well.
@@ -6,6 +6,9 @@ | |||
@include background-colors-deprecated(); | |||
@include foreground-colors-deprecated(); | |||
@include gradient-colors-deprecated(); | |||
--wp--preset--font-size--normal: 16px; | |||
--wp--preset--font-size--huge: 42px; | |||
@include font-sizes-deprecated(); |
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.
By doing this, the classes will now be :root .has-font-size-<slug>
and the specificity 020, which increases it from before (010) where we only used the class.
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.
At 669f4f0 removed them from the mixin and added below: it keeps its specificity and work like the other legacy font sizes defined below.
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.
I see the same a @jasmussen. Fallbacks seem to work just fine. Code looks good too.
I've given this a bit of a test as well. ✅ The code changes make sense. I couldn't see an easy option to display more meaningful values in the |
That had felt weird to me as well when first noticed this behavior previously and while I don't have all the context, I think it makes sense. If we select one of the available sizes/presets we only store the |
Thanks all for the quick testing. I've done some myself and this appears to work as expected. I'm going to merge this and mark it to be merged in the next beta. |
Wanted to follow up on this: with the current code, when using the segmented control it shows "undefined" labels at the top and when using the dropdown it shows only the numbers. Nik and I looked at this and he's going to prepare a new PR to pull the names from the defaults if the theme doesn't provide them. |
Sounds excellent 👌 |
Yes, to clarify the above — the theme provided a "normal" font size preset, but the preset supplied by It's no longer an issue for TT2 since we don't supply a "normal" preset, but wanted to flag it in case it's an issue. |
Thanks for reporting. I've prepared a fix at #37526 (lowers the specificity of the CSS custom properties). |
Fixes #37378
Related #37038
Testing
.has-<slug>-font-size
) or variables (--wp--preset--font-size-<slug>
) should still work as expected.