Skip to content
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

TwentyTwentyOne: set default font sizes for WordPress 5.9 #2158

Closed
wants to merge 7 commits into from
Closed

TwentyTwentyOne: set default font sizes for WordPress 5.9 #2158

wants to merge 7 commits into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Jan 13, 2022

Trac ticket https://core.trac.wordpress.org/ticket/54782
Related #2154 #2140

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.

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

  • Use the TwentyTwentyOne theme.
  • Create a post that contains paragraphs with the following content:
    • Extra Small (16)
    • Small (18)
    • Normal (20)
    • Large (24)
    • Extra Large (40)
    • Huge (96)
    • Gigantic (144)
  • Use the font size control of each paragraph to apply the font size that corresponds to its content: apply "extra small" to the 1st paragraph, etc.

The expected result is that the font size for each paragraph has the assigned value, both in the editor and front-end.

Colors

  • Use the TwentyTwentyOne theme.
  • Create a post.
  • Create the following paragraphs:
    • One with the background color black.
    • One with the background color white.
    • One with the text color black.
    • One with the text color white.
  • Apply to each of them the color corresponding with its text from the theme color palette.
  • Publish the post.
  • Via the browser devtools make sure that the colors are proper in both the editor & front.

@@ -2145,6 +2145,7 @@ pre.wp-block-verse {

:root .is-small-text,
:root .has-small-font-size {
--wp--preset--font-size--small: var(--global--font-size-sm);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if now that we have variables --wp--preset--font-size--small if it still makes sense to have --global--font-size-sm? Maybe we could remove all --global--font-size variables and use the presets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to scope down this to the minimal patch that works. Didn't want to rebuild how the theme is built to avoid falling into deep holes.

background-color: var(--global--color-black);
}

.has-black-background-color[class] > [class*=__inner-container] {
--local--color-background: var(--global--color-black, #000);
--wp--preset--color--black: var(--local--color-background);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems strange to be setting preset color black to --local--color-background. It seems more natural for a background to reference a preset than a preset referencing a background. Couldn't we set the presents globally all the in the same place:

:root {
--wp--preset--color--black: #cccc, 
--wp--preset--color--white: #cccc, 
....
}

And then never set additional presets?

Copy link
Member Author

@oandregal oandregal Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have the time to investigate how this theme is built deeply, so I'm not sure. I wanted this PR's scope to remain small, focused on fixing the issue.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes seem to be working well on my tests 👍

@oandregal
Copy link
Member Author

I'm going to close this in favor of #2233

@oandregal oandregal closed this Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants