Automattic / _s

Hi. I'm a starter theme called _s, or underscores, if you like. I'm a theme meant for hacking so don't use me as a Parent Theme. Instead try turning me into the next, most awesome, WordPress theme out there. That's what I'm here for.
http://underscores.me/
GNU General Public License v2.0
10.93k stars 3.12k forks source link

Using consistent font-sizing #1414

Closed Ismail-elkorchi closed 4 years ago

Ismail-elkorchi commented 4 years ago

This issue was reported by @jaclyntan in https://github.com/Automattic/_s/issues/1322#issuecomment-462778860 :

one thing that did confuse me with the woocommerce styles in _s is that font sizing often changes between font-size: 1em; and font-size: 1rem;. Can this be resolved and made consistent with other _s styles?

VasusMe commented 4 years ago

I am using the following font-size mixin in my vasus theme, this px <> rem fallbacks gives me perfect result on all devices.

It will give you Rem output with px fallback and if you input with px then it will give you px output with rem fallback. If you input plain number it will consider as rem and add rem at the end of number.

// Rem output with px fallback
@mixin font-size($size){
    $rem_ratio: 16px / 1rem;

    $unit: unit($size);
    @if $unit == 'rem' {
      font-size: $size * $rem_ratio;
      font-size: $size;
    }
    @else if $unit == 'px' {
      font-size: $size;
      font-size: $size / $rem_ratio;
    }
    @else {
      font-size: $size * 1rem;
    }
}
Ismail-elkorchi commented 4 years ago

This issue doesn't involve px fallbacks, though. They were just deleted in https://github.com/Automattic/_s/pull/1413. This issue highlights the fact that sometimes the font-size property uses em values, other times it uses rem values, and that they should be consistent by using either one or the other.

But, after searching in the code base, I found only one instance that uses em value. I had the impression that they were more 🤔.

VasusMe commented 4 years ago

Right, Just one instance there.

m-e-h commented 4 years ago

I think a lot of the font-size: instances could be replaced with the inherit value or removed all together. Most of the values I'm seeing in the theme are 1em/rem. Particularly in the case of 1em, isn't this the same as font-size: inherit. Which unless it's on a heading or another special case tag would be given the inherit value by the browser anyway.

Some of the non-1rem values in the theme, like the pre tag, might be better left to the normalize.css styles as otherwise they become somewhat opinionated.

And speaking of opinions, my opinion of the button: {font-size: 0.75rem;} is that it's kinda small. :smiley: Pretty sure most themes have to override that.

Ismail-elkorchi commented 4 years ago

Thanks @m-e-h for chiming in. Those are a lot of lines to remove :smiley:

I think a lot of the font-size: instances could be replaced with the inherit value or removed all together. Most of the values I'm seeing in the theme are 1em/rem. Particularly in the case of 1em, isn't this the same as font-size: inherit. Which unless it's on a heading or another special case tag would be given the inherit value by the browser anyway.

I think that specifying font-size: on the body element is sufficient as all the children will inherit this property automatically. Then, we can change font-size: only on the tags that needs too, like h1.

And speaking of opinions, my opinion of the button: {font-size: 0.75rem;} is that it's kinda small. smiley Pretty sure most themes have to override that.

I'm pretty sure that experience-based opinions aren't merely opinions.