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

TwentyNineteen: fix default presets in 5.9 #2140

Closed
wants to merge 9 commits into from
Closed

TwentyNineteen: fix default presets in 5.9 #2140

wants to merge 9 commits into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Jan 11, 2022

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

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.

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.

  • Use the TwentyNineteen theme.
  • Create a post that contains 5 paragraphs with the following content:
    • Small (19.5)
    • Normal (22)
    • Default
    • Large (36.5)
    • Huge (49.5)
  • To each paragraph apply the font size that corresponds to its content: apply small to the 1st paragraph, normal to the 2nd, nothing to the third, large to the 4th, and huge 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.

TODO

  • There's an issue in compiling the SASS variable into values when it's used by a CSS Custom Property. Solved this issue by using the raw values directly. The issue is bigger than this PR: the existing config/utils to process CSS don't compile CSS Custom Properties. I've tried to update it to do it gets complicated quickly and would require wide testing.
  • Figure out why Tested up to: 5.9 is removed when running npm run build. Fixed at 54d062c

@oandregal oandregal changed the title Fix/default presets twentynineteen Fix TwentyNineteen font sizes Jan 11, 2022
@@ -9,6 +9,22 @@ Twenty Nineteen Editor Styles

/** === Editor Frame === */

.has-small-font-size {
Copy link
Member Author

@oandregal oandregal Jan 11, 2022

Choose a reason for hiding this comment

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

Ported the font sizes here. This was a bug on its own that this PR also fixes: the theme needs to enqueue the font size classes, which TwentyNineteen didn't do, forcing Gutenberg to add inline styles to the blocks to make it work.

@oandregal oandregal changed the title Fix TwentyNineteen font sizes TwentyNineteen: fix font sizes default presets in 5.9 Jan 13, 2022
@@ -4,6 +4,7 @@ Theme URI: https://wordpress.org/themes/twentynineteen/
Author: the WordPress team
Author URI: https://wordpress.org/
Description: Our 2019 default theme is designed to show off the power of the block editor. It features custom styles for all the default blocks, and is built so that what you see in the editor looks like what you'll see on your website. Twenty Nineteen is designed to be adaptable to a wide range of websites, whether you’re running a photo blog, launching a new business, or supporting a non-profit. Featuring ample whitespace and modern sans-serif headlines paired with classic serif body text, it's built to be beautiful on all screen sizes.
Tested up to: 5.9
Copy link
Member Author

Choose a reason for hiding this comment

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

In 2a04054#diff-43320bbf5db05687019f349593821f99e7827c259d03e05867b4cd3f2a81f74d this live was added to the CSS file but should have been added here instead (and build to CSS after).

@@ -930,18 +934,22 @@

//! Font Sizes
.has-small-font-size {
--wp--preset--font-size--small: 0.88889em;
Copy link
Member Author

Choose a reason for hiding this comment

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

The current setup doesn't allow for using --wp--preset--font-size--small: $font__size-sm. I've tried to upgrade to a newer setup but ran into a few issues and couldn't. This is the raw value after compiling SCSS into CSS.

@oandregal oandregal changed the title TwentyNineteen: fix font sizes default presets in 5.9 TwentyNineteen: fix default presets in 5.9 Jan 13, 2022
@ockham
Copy link
Contributor

ockham commented Jan 21, 2022

Testing with 5.9 (by building this branch, and running it according to the instructions).

Font size Editor Frontend
Small (19.5) 19.5 19.5
Normal (22) 22 24.75
Default 22 22
Large (36.5) 37.125 37.125
Huge (49.5) 49.5 49.5

I.e. I see slight discrepancies for Large in both the editor and the frontend, and for Normal on the frontend.

@oandregal
Copy link
Member Author

Those discrepancies for normal and large are a result of how the theme is built: using px in some places and ems in other places and were present before this PR. That's an ok result.

@ockham
Copy link
Contributor

ockham commented Jan 21, 2022

Testing with 5.7, by doing the following:

On this branch:

git diff $(git merge-base trunk $(git rev-parse --abbrev-ref HEAD)) > ../default-presets-twentynineteen.patch

Then switch to the 5.7 branch, apply the patch, and rebuild as follows:

git checkout 5.7
patch -p1 < ../default-presets-twentynineteen.patch.patch
npm i
npm run build:dev

Results:

Font size Editor Frontend
Small (19.5) 19.5 19.5
Normal (22) 22 24.75
Default 22 22
Large (36.5) 36.5 37.125
Huge (49.5) 49.5 49.5

I.e. same small discrepancies on the frontend as for WP 5.9, but accurate values in the editor.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Giving tentative approval 👍 The PR seems to behave as it's supposed to as specified by the PR desc, except for minor discrepancies which seem to be okay.

The code seems to make sense to me, but I'm by no means an expert on Global Styles. It'd be great if someone who knows GS better could also give this a look 😅

@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