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

line-height should be unitless #97

Closed
frenic opened this issue Feb 10, 2017 · 1 comment
Closed

line-height should be unitless #97

frenic opened this issue Feb 10, 2017 · 1 comment

Comments

@frenic
Copy link

frenic commented Feb 10, 2017

You convert line-height to ems which result in some unexpected behavior. The recommendation is to use unitless values. Is there any reason you chose to convert to ems?

@JohnAlbin
Copy link
Owner

That is only done when the $normalize-vertical-rhythm is set to true. So if the user wants a vertical rhythm, it will set line-heights in ems (on the html element) and in rems on other elements.

However, I'm now wondering if this bit of code too-aggressively turns on this feature.

// If we've customized any font variables, we'll need extra properties.
  @if $base-font-size != 16px
    or $base-line-height != 24px
    or $base-unit != 'em'
    or $h1-font-size != 2    * $base-font-size
    or $h2-font-size != 1.5  * $base-font-size
    or $h3-font-size != 1.17 * $base-font-size
    or $h4-font-size != 1    * $base-font-size
    or $h5-font-size != 0.83 * $base-font-size
    or $h6-font-size != 0.67 * $base-font-size {
    $normalize-vertical-rhythm: true !global;
  }

You should be able to change $base-font-size and $h1-font-size without triggering the vertical rhythm feature. But all the other variables listed above are only needed if you want a vertical rhythm.

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

No branches or pull requests

2 participants