AlaskaAirlines / AuroDesignTokens

Abstract UI atomic values to support the Auro Design System.
https://auro.alaskaair.com/getting-started/developers/design-tokens
Apache License 2.0
19 stars 7 forks source link

Line height tokens should not have units #54

Closed geoffrich closed 3 years ago

geoffrich commented 3 years ago

Describe the bug The tokens used for line height in WCSS use rem units. Per MDN, line height values should be unitless.

--auro-text-body-height-xs: 1rem;
--auro-text-body-height-sm: 1.25rem;
--auro-text-body-height-default: 1.5rem;
--auro-text-body-height-lg: 1.625rem;

WCSS uses auro-text-body-height-default to set a default line height, effectively setting line height to 24px regardless of font-size. At font sizes larger than 24px, the text will start to run into each other.

https://github.com/AlaskaAirlines/WebCoreStyleSheets/blob/6fcc2f36af78655186cc501e5d31d6aa4924173b/src/_essentials.scss#L66

To Reproduce See this CodePen for an example.

The second paragraph is auro-size-xl and the text starts to run into itself.

Expected behavior Line height does not have a unit

Screenshots If applicable, add screenshots to help explain your problem.

image

Desktop (please complete the following information):

Additional context We ran into this in one of our sites and had to override the line-height to 1.5 in our styles.

blackfalcon commented 3 years ago

This is a debate to take back to the design team. When the latest gen of tokens was created this was a debate between myself and the lead designer at the time.

In the end, the use of units was not accidental.

Is there a situation where the use of these tokens in production is causing issues with the development of UIs?

There is a growing discussion about the use of typography and this is not something engineering can address alone.

Who is the designer you are working with? I'd suggest raising this issue up to design leadership.

This issue has been reassigned and re-labeled accordingly.

geoffrich commented 3 years ago

I can understand why we'd want to define line height units on our height tokens for heading levels and such, so I retract my request to remove units from the height tokens. However, I think setting a default line-height value with a unit on body has too many side effects. Since line-height is inherited, all children will have a default line-height of 24px regardless of their font size, which causes the issues I screenshotted above. 1.5 would be a more sensible default and reduce unintended consequences. Users would then be free to override the line-height on a per-element basis.

Maybe this should be a WCSS issue instead?

blackfalcon commented 3 years ago

Maybe this should be a WCSS issue instead?

Yes. This is an issue regarding the expectation of type with design. Really, this is not even a WCSS issue, but something that needs to be proposed with the designers.

I will mention this to the UI designers and see if there are things we need to address.