Use named font sizes instead of values in px #2533
Conversation
Aljullu
added
status: needs review
focus: components
|
Size Change: +63.7 kB (3%) Total Size: 2.06 MB
ℹ️ View Unchanged
|
| @@ -237,7 +237,7 @@ const Edit = ( { attributes, setAttributes, debouncedSpeak } ) => { | |||
| ) } | |||
| </p> | |||
| <Button | |||
| className="wc-block-attribute-filter__add_attribute_button" | |||
| className="wc-block-attribute-filter__add-attribute-button" | |||
There was a problem hiding this comment.
Not related to this pull, but I took the chance to change this class name so it follows BEM convention.
There was a problem hiding this comment.
This is released code though, so we might want to make sure it's noted in the dev notes/changelog for the next release.
There was a problem hiding this comment.
Those classes (wc-block-attribute-filter__add-attribute-button and wc-block-price-slider__add-product-button) are only used in the editor, they refer to the button in this placeholder:
In that case, when an element is only displayed in the editor, do we need dev notes too?
There was a problem hiding this comment.
We shouldn't do - this is internal.
|
|
||
| p, | ||
| .wc-block-product-metadata { | ||
| line-height: $gap; | ||
| line-height: 1.375; |
There was a problem hiding this comment.
These line heights needed to be relative.
There was a problem hiding this comment.
It feels like these should have their own mixin helper.
| @mixin font-size($sizeName) { | ||
| font-size: map-get($fontSizes, $sizeName) * 1em; | ||
| } |
There was a problem hiding this comment.
Why the switch from rem to em here? Wouldn't we want to keep rem so that the scale is relative to root? I understand that in many places being refactored to use this, em was the unit used. However, that makes me wonder if we should allow the passing in of an argument to use rem or em? As it stands, this means all existing usages of font-size are now switched from rem to em right?
There was a problem hiding this comment.
We saw rem units were not playing well with many themes because they set <html> font size to something very small and then make it bigger in the <body>. One example of this is Twenty Twenty:
https://github.com/WordPress/twentytwenty/blob/master/style.css#L117
In pb0Spc-Cv-p2 I proposed switching to em to fix this. Considering rem can give so different results depending on the theme, I think we should avoid it when possible.
There was a problem hiding this comment.
👍 EM is relative to the current content too so it should fit better within themes. Not all themes use the html/body class for the base size.
| @@ -5,7 +5,7 @@ | |||
| display: flex; | |||
| font-size: inherit; | |||
| font-weight: bold; | |||
| min-height: rem(48px); // 16px * 3 = 48px, same height as text-input | |||
| min-height: 3em; | |||
There was a problem hiding this comment.
Why not using the new em function (em(48px)) (assuming there's good reason to switch from rem to em)?
There was a problem hiding this comment.
In pb0Spc-Cv-p2's comments, @mikejolley raised the concern that it might be confusing for developers to write px units but see they are em's in the resulting CSS. In this concrete case, if you are debugging a theme with a font size of 14px and want to know how high that element will be, I think it's easier to rationale "3em = 14*3 = 42" than thinking "48 / 16 = 3, 14 * 3 = 42".
I still think the em() mixin makes sense to convert px variables to em values. But in other cases it might be more intuitive to use em than px. Does that make sense?
| @@ -92,7 +93,8 @@ | |||
|
|
|||
| label { | |||
| @include reset-typography(); | |||
| @include font-size(16px, 22px); | |||
| @include font-size(regular); | |||
| line-height: 1.375; // =22px when font-size is 16px. | |||
There was a problem hiding this comment.
We originally had line-height included as an argument of the font-size mixin, why did you choose to extract it back out?
There was a problem hiding this comment.
The reason is similar to why I removed several calls to the rem() mixin. You can see this comment for an example on how things might start getting difficult if we use px values when they are in fact em under-the-hood.
For the line-height, my reasoning was a bit the same. If the font sizes are named, does it make sense the line-height being in px? What if we change the large for size from 1.25 to 1.5 at some point? If line height are specified in px, we would need to update all of them while if they are unitless, they will automatically adapt.
I agree 1.375 looks like a magic number, here. That's why I added a comment next to it explaining where it comes from. Another option would be to create SCSS variables for different line heights so we can reuse them and we know where they come from. Do you think that would make it easier?
| background-color: #fff; | ||
| box-shadow: none; | ||
| color: $input-text-active; | ||
| font-family: inherit; | ||
| font-weight: normal; | ||
| height: rem(48px); | ||
| height: 3em; |
| font-family: inherit; | ||
| margin: 0; | ||
| box-sizing: border-box; | ||
| height: rem(48px); | ||
| height: 3em; |
| @@ -237,7 +237,7 @@ const Edit = ( { attributes, setAttributes, debouncedSpeak } ) => { | |||
| ) } | |||
| </p> | |||
| <Button | |||
| className="wc-block-attribute-filter__add_attribute_button" | |||
| className="wc-block-attribute-filter__add-attribute-button" | |||
There was a problem hiding this comment.
This is released code though, so we might want to make sure it's noted in the dev notes/changelog for the next release.
|
|
||
| p, | ||
| .wc-block-product-metadata { | ||
| line-height: $gap; | ||
| line-height: 1.375; |
There was a problem hiding this comment.
It feels like these should have their own mixin helper.
| @@ -67,11 +67,11 @@ | |||
| border: none; | |||
| cursor: pointer; | |||
| background: none; | |||
| padding: 0 rem(8, 13); | |||
| padding: 0 0.5em; | |||
There was a problem hiding this comment.
curious why you're not using em() for all these values?
There was a problem hiding this comment.
Like before, I think using em() (or rem() as it was until now) makes this code harder to understand.
In fact, there is a curious bug here. That element had 13px font-size, and we needed a padding of 8px, so it was rem(value: 8, base: 13). But in #2498 I removed the font-size 13px but forgot to update this mixin. 🤦♂️ So instead of 8px now the padding is 9.8px (=8/13*16).
To avoid the need to do all this kind of calculations, I think we should get used to use em values when possible.
|
Thanks for the review @nerrad! There were some comments which were very similar so I tried to only answer one of each to focus the discussion there. The goal of this PR is to make writing and reading styles easier for us. I tried to explain the rationale for the changes, but none of them is a hard opinion. I acknowledge what I see as being harder/easier might differ from somebody else, so I'm open to discuss all changes individually to make it easier for the team to modify styles. |
Aljullu
force-pushed
the
fix/named-font-sizes
branch
from
041acee to
46ac705
Compare
| "smaller": 0.75, | ||
| "small": 0.875, | ||
| "regular": 1, | ||
| "large": 1.25, | ||
| "larger": 2, |
There was a problem hiding this comment.
I think smaller small... would fit well with sizing but not much with font sizes, as they provide no obvious context, I would possibly suggest something like heading caption body and so on, they provide more context.
see examples in:
However, there’s no size fit all, other opinions on this:
- Font size naming convention in designer news.
- Naming typography patterns in CSS
- Typography in Design Systems
- CSS utility classes for sizing and naming conventions
Why do I care so much about such a small thing?
Because those names will in the future be a public API for our blocks, both in global styles, extending and our style guide, getting them right is somehow a bit important.
This is by no means a blocker, we can address this later, we can merge this, or we can address it now, your call.
There was a problem hiding this comment.
Loads of good ideas here, thanks for the feedback @senadir. So if I understand you correctly, you are proposing to define a heading named font size, for example, so if we ever get a --wp-gs-heading-font-size custom property (or whichever system global styles ends up using), we could easily update the font sizes of all headings to use the style from global styles. Is that correct?
While I really like this idea, I have a couple of thoughts about this:
- Currently, if we have a heading or a button that should inherit the theme font size, I think it shouldn't have any font size defined in its CSS and instead rely on the inherited value. Said in other words: all the font sizes introduced in this PR are variations of the body text font size. If an element must honor the theme styles (for example, an
<h2>needs to render as all other<h2>are rendered in that theme), it should just inherit the font size. At least, until global styles land. - In parallel to this, I think it's a bit too early to know how the final implementation of global styles will be and which specific values we will be able to use. In the examples I have seen, we would be able to read the heading, body and button font sizes, for example. But what about caption, for example? There are dozens of HTML elements and I don't think global styles will expose specific styles for all of them, so my position with this would be to wait a bit more to see how global styles evolve before embracing this.
Let me know how that sounds! In any case, I agree we shouldn't block this PR because of that since it can be tackled in a follow-up.
There was a problem hiding this comment.
Well, I'm not suggesting 1:1 with GS, global styles could be an example but it’s still early for it, my point is that naming font sizes large, small... provides no context to us as developers or to third-party developers, font size depends on context, you will probably never have a heading that is 10px, having semantic names will make sure we're using correct tags and correct placement.
Global styles could be another layer of abstraction above this if we don't want to expose everything, I don't disagree on the values or how they're used, mainly the naming and how intuitive it can get.
There was a problem hiding this comment.
I think smaller small... would fit well with sizing but not much with font sizes, as they provide no obvious context, I would possibly suggest something like heading caption body and so on, they provide more context.
Isn't the point here to have a flexible system where the content is unknown? Is we based it on selectors we could run into code smell where the design uses one size, and we're forced to use a named size that doesn't match our usage.
There was a problem hiding this comment.
Isn't the point here to have a flexible system where the content is unknown
A design system still needs to follow certain constraints
If we based it on selectors we could run into code smell where the design uses one size, and we're forced to use a named size that doesn't match our usage.
We're not basing it on selector but on context, a body-sized text could be any element, not strictly applied to the body tag, by having those rules, a designer or a developer will ask the question "Is this a heading? if not, why is the size the same as other headings within this design", if we're going to communicate and emphasize importance and context via fonts sizes, variables should be named accordingly.
There was a problem hiding this comment.
@senadir Not all headers are equal. There are also times where a heading may be styled differently right? I think the defined sizes work better for our use case. We have a base and everything else is just a slight variation of that. Perhaps we just need inline docs to identify which are most commonly used for headings?
There was a problem hiding this comment.
If by headings we are referring to <h1>, <h2>, etc., I don't think we need to establish a relationship between them and specific font sizes, in several occasions we need to display headings as if they were regular text.
In the other hand, for headings that need to honor the font size defined by the theme (I'm thinking on the Filter block titles, for example) I think not setting any font size on our end and letting styles cascade is the best option.
|
|
||
| // Maps a named font-size to its em equivalent. | ||
| @mixin font-size($sizeName) { | ||
| font-size: map-get($fontSizes, $sizeName) * 1em; |
There was a problem hiding this comment.
Just a small question on mixin usage. Are there any advantages to the mixin over variables?
i.e.
@include font-size(larger);
vs
font-size: $larger;
There was a problem hiding this comment.
A mixin gives us more control, so the idea is that it will allow us to easily migrate all font sizes to support global styles in the future. There is an example here: #2403 (comment).
That example is a bit old (it uses px→rem instead of named→em) and global styles are still in development so its shape might be different from the one in that example, but in general I think having all font sizes in a mixin will eventually make it easier to embrace global styles.
However, I acknowledge that with search & replace we can easily switch between @include font-size(larger); ←→ font-size: $larger;, so I don't have a strong preference on any of them other than in the future we might be forced to use mixins.
There was a problem hiding this comment.
Can we support both? Readability I prefer standard font-size. In no means a blocker though :)
There was a problem hiding this comment.
Hmm, I'm a bit on the edge with this. While I don't have any strong preference between both systems, I would really prefer that we stick with one. It really simplifies things when searching for code.
I will merge this as-is for now, and we can migrate in a follow.
Co-authored-by: Mike Jolley <[email protected]>
There was a problem hiding this comment.
I went through cart and checkout and didn't spot any visual regressions so that looks good 👍
Code looks okay too - my only comments are that:
- For simple font sizing, I prefer the non-mixin version for readability (so using the variable directly).
- This comment regarding line heights I think is a good use case for a combined font-size/line height mixin. Because if we base the line height on a consistent height (base), or a multiple of base, we'll have better vertical alignment of side by side elements.
So for example, the regular size is 1, and lets assume we want line height to always be 1.5.
For the font size large, which has size 1.25, we'd make the line height 1.5/1.25 = 1.2.
For the font size larger, which has size 2, we'd need a multiple of the base size. So probably 2xbase which would make the line height 1.5 for this element too.
I'm happy for line heights to be worked on in a follow up anyway - I'm approving the current work but it's up to you if you want sign off from everyone else here too :)
| @@ -237,7 +237,7 @@ const Edit = ( { attributes, setAttributes, debouncedSpeak } ) => { | |||
| ) } | |||
| </p> | |||
| <Button | |||
| className="wc-block-attribute-filter__add_attribute_button" | |||
| className="wc-block-attribute-filter__add-attribute-button" | |||
There was a problem hiding this comment.
We shouldn't do - this is internal.
|
Thanks for all the reviews! I created a follow-up to explore ways to simplify line-height property: #2584. |
Part of #2458.
This PR updates the
font-sizemixin to use named font sizes instead of px values.The font size names are
smaller,small,regular,largeandlarger, I chose this ones so it's consistent with the$gap-xyzvariables we have for margins/paddings.Searching & replacing seemed to work quite well in most cases. 👌 But I did some manual changes as well, I tried to comment all of them inline with the code. I didn't modify yet the All Products and filter blocks until we agree on how to map their font sizes to the named ones (#2536).
How to test the changes in this Pull Request: