-
Notifications
You must be signed in to change notification settings - Fork 320
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
Convert line-height from pixel values #848
Conversation
src/helpers/_typography.scss
Outdated
|
||
// Convert line-heights specified in pixels into a relative value, unless | ||
// they are already unit-less (and thus already treated as relative values) | ||
@if not unitless($line-height) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you set your line-height to something other than 'px' would it this fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, because Sass doesn't know how to e.g. divide pixels by em.
We've got a few options…
- let Sass' error handling catch it
- detect if the units do not match and do nothing (output line-height as defined in the map)
- detect if the units do not match and throw our own error
Not sure what the best option is. Can you think of cases where you'd intentionally want to use define font-size and line-height in different units?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the only case I'm thinking of is this could be confusing for people making their own font maps, but if it's clearly documented maybe it'll be fine.
@NickColley I've made the line-height stuff into its own function and attempted to make it handle those edge cases better. Can you please re-review? |
Good use of tests 👍 |
changelog needed for this? |
Rather than having to declare the line-heights as relative values (such as 1.5) in the font map, allow them to be specified in pixels and convert them to relative unitless values within our typography mixin. This provides a much clearer way to define line heights without the need for comments explaining how the values are calculated, whilst retaining the benefits of using relative unitless line-heights.
By doing the font-size override first we also end up converting overridden line-heights into relative numbers.
66596b8
to
f5d5479
Compare
Rather than having to declare the line-heights as relative values (such as 1.5) in the font map, allow them to be specified in pixels and convert them to relative unitless values within our typography mixin.
This provides a much clearer way to define line heights without the need for comments explaining how the values are calculated, whilst retaining the benefits of using relative unitless line-heights.