reactist
reactist copied to clipboard
style: [internal] Update headers to align size, weight and line-height with product library styles
- 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.
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?
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.
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.
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?
More so for @anaf 😃 and yeah it would be great if Figma and the implementation matched once we have a solution 👍
@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.
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…
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.
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.
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 :)
Closing after discussion held here with new deliverables, let's create a new PR for this when addressed.