Skip to content
This repository has been archived by the owner on Jan 15, 2019. It is now read-only.

Fix font size variables to resolve to exact pixels #318

Conversation

2ndkauboy
Copy link
Contributor

Fixes #164

Username: Kau-Boy

@2ndkauboy 2ndkauboy changed the title fix/font-size-variables-exact-pixels Fix font size variables to resolve to exact pixels Oct 25, 2018
@kjellr kjellr self-requested a review October 25, 2018 15:01
@2ndkauboy 2ndkauboy force-pushed the fix/font-size-variables-exact-pixels branch from 5afce22 to 417ef98 Compare October 25, 2018 21:51
@kjellr
Copy link
Collaborator

kjellr commented Oct 26, 2018

Interesting! This seems promising.

Question for you: From what I can tell, the new font size variables here should equate to exact pixel values (as shown in this CodePen). But they're not doing that for us. Is that because of rounding/truncating of decimal points in our Sass compiler? If so, I wonder if we can tweak that. If another few decimal places will mean we have more exact pixel sizes, I'm all for it.

Regarding the new type scale: The new font sizes here sound reasonable, but I'll likely want to give this a more in-depth review over the weekend. I know we'd been using ~37px text in places that will be either much larger or much smaller with this new scale. Also, this PR helped me notice that we have an unintended ~25px font size used in many areas of the site (the site header for example). When we switch to this PR, that text is sized down to 22px. 😄 In general, I may want to make some slight adjustments to the font sizes to smooth these things out.

In any case, I'll take another in-depth look at this over the next couple days. I'll also encourage @allancole to take a look too.

Thanks again for the PR! This is really helpful.

@2ndkauboy
Copy link
Contributor Author

In em, many values cannot round to exact px values, given a base of 22px. If we would use a base on the html of 10px we could set the base for the body to 2.2em and all other values would perfectly round to values such as 4.8em instead of 2. 18182 with the base value 22px.

@kjellr
Copy link
Collaborator

kjellr commented Oct 26, 2018

In em, many values cannot round to exact px values, given a base of 22px. If we would use a base on the html of 10px we could set the base for the body to 2.2em and all other values would perfectly round to values such as 4.8em instead of 2. 18182 with the base value 22px.

Right, right. I guess I was just puzzled why the CodePen does produce exact pixel values for me, whereas this doesn't. It must be the rounding. CodePen equates $font__size-lg to be 1.2727272727em, while our CSS compiler outputs 1.27273em.

As you mention, we can't do much better in this case, short of assigning the base font size to be something nice and round like 10px (which I think has the potential to cause issues somewhere, right?)

@2ndkauboy
Copy link
Contributor Author

I am currently at a conference, but I try to create a Code Pen solving the issue of rounded values, which you also take the issue #62 into account.

@2ndkauboy
Copy link
Contributor Author

2ndkauboy commented Oct 27, 2018

@kjellr: This could be an solution using rem with a 1em = 10px on the html tag using font-size: 62.5%: https://codepen.io/anon/pen/qJvrNL

To support older browsers, we could use a simple "rem to px" PostCSS script, like this one: https://github.com/robwierzbowski/node-pixrem

@joyously
Copy link

font-size: 62.5%

Before you do that, please reread what I wrote in the other font ticket about using 100%.
And I've seen a lot of themes that use Bootstrap with that in there and then they have issues with nested things being tiny (because they use rem wrong).

@2ndkauboy
Copy link
Contributor Author

2ndkauboy commented Oct 27, 2018

I am sorry @joyously but I don't see the issue here. Using rem (and not em) will not result in things being tiny when nested. The body still has the default size of 22px equivalent. I can resize the default font in Chrome using both optione (zoom the whole page or only setting the font-size to another browser default). Everything just resizes perfectly. I also can't see any difference, when using 100% for the html. Can you give us an example, where these definitions would fail?

@joyously
Copy link

I never said they would fail. I said please don't use anything but 100% (or larger) on html. It is still an accessibility issue, and it should not be used to get pixel division correct.
In an ideal world, pixels would never be used for fonts, because you don't know what your base is. Giving the user easier-to-use controls for font size in the editor opens a Pandora's box.. It really is not such a good thing.

The examples I have seen, reviewing many themes submitted to the repository, is that Bootstrap uses the 62.5% rule and then there is something (maybe in _s) that uses rem. A lot of authors don't test everything, so they don't see it, but I would see it in my test data in the same place every time (some nested thing that should have used em, but had rem). I dislike rem, because everything's default is already working fine. Nothing really needs a font size change, and when you change it using rem it is difficult to make it work for all nested things. If a font size change is needed, it should be a class on the container, to affect children equally, using percent or ems. That way, there are no nesting font size problems. (I know <big> and <small> should be on the elements, using %.)

@2ndkauboy
Copy link
Contributor Author

2ndkauboy commented Oct 27, 2018

I still don't see the a11y issue here. The combination of 62.5% on the html and 1rem on the body would result in 100% of the users default size (which is usually 16px for most browser).

From that size, a designer can use em, %, and other relative sizes and they would all scale the font without any a11y issue. I couldn't find a single blog post about font-sizes and a11y where a base size of something different than 100% is an issue. They all just say, that the font-size hasto be resizable.

If we do want to use 22px as the default size of the body font-size (no matter in which unit we accompish that), useing 100% on the html would not make it possible to have all the other sizes rounded to exact pixel values.

@2ndkauboy
Copy link
Contributor Author

Oh, do you acutally want to say, that we should not use a font-size of 22px but of 16px (100%)? Then I know were the miscommunication might come from :)

@2ndkauboy
Copy link
Contributor Author

OK, maybe I found a good solution: https://codepen.io/anon/pen/NOJaxY

The html font-size is at 100%, the body font-size is at 22px and all the other values round perfectly without any rounded periodic numbers.

@joyously
Copy link

joyously commented Oct 28, 2018

Your codepen is better, but not quite there. There is no need for a rule for the body, and by having one, it sort of eliminates the value of using 100% on html.
Why would you make the base size equivalent to extra small? Shouldn't the base be in the middle, and the user could make smaller or larger text? (I see that you made medium the same number as base, but body should not have base, so it would be extra small.)

Using rems on these sizes makes them set that size, regardless of nesting, but if anything with a % or ems is used to change the whole container, these will not change with them. Consider blockquote with nested text that has one of these. (Of course, you'd have the same problem if you tried to use pixels.)
Edit: Have you ever seen http://css-discuss.incutio.com/wiki/Using_Font_Size

@kjellr
Copy link
Collaborator

kjellr commented Oct 28, 2018

Thanks, @2ndkauboy! Your latest CodePen looks like a definite improvement over what we have. Much cleaner than the current solution. Mind spinning up a PR with that so we can try it out in context? I think it'll work fine, but I want to make sure these correctly override some of Gutenberg's font size values.

Thanks for continuing to push this! Your work is really appreciated. 🙌

@joyously
Copy link

Hey, they are doing a contributor day in Philadelphia and going through all the Gutenbugs. One mentioned was WordPress/gutenberg#11043 which is saying that it doesn't work...

@2ndkauboy 2ndkauboy force-pushed the fix/font-size-variables-exact-pixels branch from 417ef98 to 0d74a19 Compare October 28, 2018 20:12
@2ndkauboy
Copy link
Contributor Author

@kjellr: As there have been a lot of commit since opening this PR and it was conflicted, I updated it with the code from the latest CodePen.

@2ndkauboy 2ndkauboy force-pushed the fix/font-size-variables-exact-pixels branch from 0d74a19 to 934d8d4 Compare November 1, 2018 08:45
@kjellr
Copy link
Collaborator

kjellr commented Nov 13, 2018

@2ndkauboy, just wanted to let you know I haven’t forgotten about this one. This change will likely have some cascading effects on font styles throughout the site (I’m specifically thinking about the editor styles), so I doubt we’ll be able to get it in by RC1. Especially since we’ll need to continue focusing on finding/fixing bugs for that release.

Although this is lower priority, I do still have hope we’ll get this tested and updated somehow. Keeping my fingers crossed!

@2ndkauboy
Copy link
Contributor Author

OK, no problem. Should I fix the conflict or wait until we proceed with this issue?

@kjellr
Copy link
Collaborator

kjellr commented Nov 13, 2018

Let's hold off for now — I think fixing existing issues and general testing is more important at the moment. 👍 Thanks!

@kjellr
Copy link
Collaborator

kjellr commented Jan 10, 2019

Hi @2ndkauboy! Since this PR is stale, I'm not going to migrate it over to Trac alongside the new ticket:

https://core.trac.wordpress.org/ticket/45899

Thanks for your work on this, and I hope we can get it sorted out in the future.

@kjellr kjellr closed this Jan 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants