-
Notifications
You must be signed in to change notification settings - Fork 165
Fix font size variables to resolve to exact pixels #318
Fix font size variables to resolve to exact pixels #318
Conversation
5afce22
to
417ef98
Compare
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 ~ 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. |
In |
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 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 |
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. |
@kjellr: This could be an solution using rem with a To support older browsers, we could use a simple "rem to px" PostCSS script, like this one: https://github.com/robwierzbowski/node-pixrem |
Before you do that, please reread what I wrote in the other font ticket about using 100%. |
I am sorry @joyously but I don't see the issue here. Using |
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. 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 |
I still don't see the a11y issue here. The combination of From that size, a designer can use If we do want to use |
Oh, do you acutally want to say, that we should not use a font-size of |
OK, maybe I found a good solution: https://codepen.io/anon/pen/NOJaxY The |
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. 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.) |
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. 🙌 |
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... |
417ef98
to
0d74a19
Compare
@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. |
0d74a19
to
934d8d4
Compare
@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! |
OK, no problem. Should I fix the conflict or wait until we proceed with this issue? |
Let's hold off for now — I think fixing existing issues and general testing is more important at the moment. 👍 Thanks! |
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. |
Fixes #164
Username: Kau-Boy