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

feat(type): use unitless line-heights #6714

Merged

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented Aug 24, 2020

Closes #6711

Updates our type package to use a unitless line-height value

Changelog

Changed

  • Modified the scss and js type scale files
  • Instead of calling a carbon--rem() to set the line-height, we just set it to a unitless value
  • Typos

Testing / Reviewing

Ensure the values match the website documentation, with the exception of heading-03. An issue has been made for that here carbon-design-system/carbon-website#1604

@netlify
Copy link

netlify bot commented Aug 24, 2020

Deploy preview for carbon-elements ready!

Built with commit 45d91b2

https://deploy-preview-6714--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Aug 24, 2020

Deploy preview for carbon-components-react ready!

Built with commit 45d91b2

https://deploy-preview-6714--carbon-components-react.netlify.app

@joshblack
Copy link
Contributor

The changes look good code-wise, although from the percy snapshots it seems like there might be some unexpected line height changes? I'm not sure how the line-height value was computed but for productive-heading-06 it seems like the font-size is 42px and the line-height was set to 3.125 which I believe gives us the 131.25px value line-height in that story (42px * 3.125 = 131.25)

I think the value of line-height should be computed by proportion instead of relative to the base font-size, so instead of 50px / 16px = 3.125 it'd be 50px / 42px = 1.19 for the line-height value. What do you think @tw15egan? 👀

@tw15egan
Copy link
Collaborator Author

@joshblack Ah yeah that would explain the differences... so would we need to compute that for all of them? i.e. body short 01 would be 18px / 14px = 1.285?

@joshblack
Copy link
Contributor

@tw15egan yeah! Exactly.

@tw15egan tw15egan force-pushed the type/unitless-line-height branch from fcbe45c to d332519 Compare August 26, 2020 18:35
@tw15egan
Copy link
Collaborator Author

tw15egan commented Aug 26, 2020

@joshblack updated. After making these changes, I noticed there were still 29 diffs in Percy. Seems like Chrome rounds 15.94px DOWN to 15px, which was throwing off any line heights that were set to 1.33. As a result, I bumped any 1.33 values up to 1.34 👍

@tw15egan tw15egan force-pushed the type/unitless-line-height branch from 6c4a519 to 854304e Compare August 27, 2020 17:07
@tw15egan tw15egan force-pushed the type/unitless-line-height branch from bd22b5c to 86a470d Compare August 27, 2020 19:37
@kodiakhq kodiakhq bot merged commit 28b6d91 into carbon-design-system:master Aug 27, 2020
@tw15egan tw15egan deleted the type/unitless-line-height branch April 28, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type styles using hard coded line heights
3 participants