reactist icon indicating copy to clipboard operation
reactist copied to clipboard

style: [internal] Update headers to align size, weight and line-height with product library styles

Open nats12 opened this issue 2 years ago • 10 comments

  • A follow-up from https://github.com/Doist/reactist/pull/640 which was closed

Short description

This PR aligns the default styles for the Heading (h1, h2, h3) with the styles from the product library. More specifically:

  • font-size
  • font-weight
  • line-height [^1]

Something for the reviewer to keep in mind - I'm concerned about how "aggressive" this change is and the impact it will have in our products as we are shifting heading sizes. I tried to contain this impact and keep it to a minimum. There are certain header variables that should be renamed, for example, to introduce the new 26px font-size in between -large and -xlarge. But refrained from doing this. I'm still not sure on how we plan to align but I felt an aggressive approach was risky here so took a gradual one.

[^1]: Each of the headers in the product library has different line heights depending on the font, I wasn't sure how to go about this so I went for the Segoe line-height as the default one. Thoughts?

PR Checklist

  • [ ] Executed npm run validate and made sure no errors / warnings were shown
  • [ ] Described changes in CHANGELOG.md

Versioning

New default Heading sizes, weights and line-heights to align Reactist with the product library. To be released as soon as it's merged.

nats12 avatar Apr 06 '22 15:04 nats12

The difference in line height looks pretty drastic in Figma even if the font themselves only differ slightly in height. https://github.com/orgs/Doist/teams/design-hero is it intentional?

For all the header sizes we were using the font default line-height. Is it possible to make if dependent on the font? Or you don't think that is a good idea in CSS?

anaf avatar Apr 12 '22 17:04 anaf

Is it possible to make if dependent on the font? Or you don't think that is a good idea in CSS?

@anaf it's possible but it would make our CSS quite complex and hard to maintain. Ideally we would only have styles changing depending on the type of header (h1, h2 etc). @frankieyan what are your thoughts? The only way I can imagine this is to have custom classes for each font and then have 3 separate types of headers for each font with their respective styles (line-height etc)? Something to also consider is that if I'm not mistaken, Reactist still doesn't support applying different fonts to components on the fly, we have a single font-family style with fallbacks so we'd need to introduce that as well.

nats12 avatar Apr 13 '22 07:04 nats12

AFAIK it's not possible to detect which font is available and is being rendered, and it's only exposed in the browser devtools. The only available option would be to detect the user's OS, but I don't think the difference in line-height is enough to justify doing it.

Can we start small and use a system that can be applied to both fonts? If possible, using a multiplier based on the font size is the most recommended.

frankieyan avatar Apr 13 '22 09:04 frankieyan

Can we start small and use a system that can be applied to both fonts? If possible, using a multiplier based on the font size is the most recommended.

@frankieyan was this question for me or Ana? :) Would this change what's defined in the Figma file?

nats12 avatar Apr 13 '22 10:04 nats12

More so for @anaf 😃 and yeah it would be great if Figma and the implementation matched once we have a solution 👍

frankieyan avatar Apr 13 '22 10:04 frankieyan

@frankieyan sounds good 👍🏼 In my latest commit I've addressed your other comments (deprecating size and removing redundant variables). I will wait for Ana's comments on the above before moving on to changing the line heights.

nats12 avatar Apr 13 '22 10:04 nats12

Ok, one more question before I can provide an answer, and then we can update figma/code. How are we setting the font in code? %, rem, px, pt, etc…

anaf avatar Apr 13 '22 20:04 anaf

How are we setting the font in code? %, rem, px, pt, etc…

We're using px for font sizes, but this is something I've been meaning to bring up. From your POV, is there a preference? Because I'd very much like to move towards using rem, but the default values in Figma from which we departed implementing the design system were expressed in px, if I recall correctly.

gnapse avatar Apr 14 '22 11:04 gnapse

Because I'd very much like to move towards using rem, but the default values in Figma from which we departed implementing the design system were expressed in px...

Yes. AFAIK Figma doesn't support rem out of the box. There's this plugin that can be used to convert px to rem though.

@anaf I'm not sure what you have in mind, but let me know if I can help in any way.

tsamoudakis avatar Apr 20 '22 14:04 tsamoudakis

Hey @anaf, just checking in to see if there have been any updates on this? :) let me know once everything is updated on Figma so I can update the code in this PR :)

nats12 avatar May 13 '22 11:05 nats12

Closing after discussion held here with new deliverables, let's create a new PR for this when addressed.

nats12 avatar Sep 13 '22 18:09 nats12