-
Notifications
You must be signed in to change notification settings - Fork 10
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
Update typography patterns to use unitless line-heights #296
Conversation
3xl: 42px, | ||
4xl: 48px, | ||
5xl: 56px, | ||
2xs: 1.1428, |
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.
@danalvrz could you put a comment to hint the final value? :)
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.
btw, if we change the line heights, it's a breaking change.
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.
@danalvrz could you put a comment to hint the final value? :)
@sneridagh the final value is always relative to the font size used.
Usually, there aren't as many line-height values as there are defined
in this project, for instance, bootstrap has only 4 values: 1.2, 1.25, 1.5, 2
https://github.com/twbs/bootstrap/blob/main/scss/_variables.scss#L629
Usually, you have a body text of 1.5, 1.25 for headings, and 1.2 for big headings.
In your design, you have a lot more values from 1 to 1.428
Let's take introduction text as an example of why the value changed from:
xl to 3xl
L - font size 24px
in figma you have:
font-size: 1.5rem;
line-height: 2.0625rem;
16 * 1.5 = 24px
16 * 2.0625 = 33px
33 / 24 = 1.375
this is why @danalvrz changed from xl which was 33px static to 1.375 which
is not.
If I change the l font-size value to 26px, the 1.375 value will keep the line height proportion
which will result in 35.75px.
You can add comments about previous values but in general the line heights are about
proportions and not static values.
I hope I made some sense to you, your request can still be fulfilled if it makes it
easier for your devs to think of what to use.
I've been looking into it, and see the state of the art and recommendations about the subject. To be honest, I thought that when zooming it would be all borked out, and it is not the case. https://www.aleksandrhovhannisyan.com/blog/dont-use-a-fixed-line-height/ The thing is that it seems that nowadays browsers adjust the ratio on zoom also, so unit-less values are not that crucial. The example in https://developer.mozilla.org/en-US/docs/Web/CSS/line-height does not apply to us, because we are explicitly setting that per typography tag, not globally. Our design is very specific about typography, and it's curated to the last intents. The browsers do a good job nowadays with zooming, and the values are set in the typography tag itself, very granular and specific, so this is not a problem to my eyes. In lieu of simplicity, I'd not touch this and leave the values as it is. If anybody needs to adjust the typography, they will have to override the typography variables anyways, so they will have to adjust the line-heights manually on its own. If they want to use unit-less vars, it's their choice. |
@sneridagh it's a bit ironic that the dude that says just use static values is using for his site unitless line-height :)) Given the fact that the theme hardcodes the HTML font size and does not allow the user to set what is the default font size it's true that it makes sense to keep the static font size and line-height making things easier for the developer to implement the pixel perfect values or to copy paste from figma dev mode if set to pixels. If the values from the $font-sizes and $line-heights are used then other users can make changes however there are still instances in your theme where there are static values used directly on elements although the values can be used from the mapping or one of the mixins could be used. I guess as in programming an anti-pattern can turn into a design pattern if you want some strict outcome that should not modified by the user somehow so yes this pull request should not be merged and things kept as they were before. |
@ichim-david it does not says either that do not use them at all because it's bad :) The values are in variables, one can change them in their projects to their liking, so I'd say let's leave it as it is. @danalvrz please state if some of the naming improvements that you found in your way are worth to keep. |
@sneridagh I think it was a good exploration, but it seems to make sense for things to stay how they were for now. I don't think we should make any naming changes either. |
Closing this PR, since it was decided to keep the line-heights with unit values. |
Fixes #273
@sneridagh four decimals are the minimum to achieve the correct heights for some designs, but we could always round them if we don't like that.
I also added a
blockquote()
mixin because I noticed theblockquote
elements were using SemanticUI styles. As a result, their font size is now bigger, but in fact that is how it is specified on the design spreadsheet, we can always adjust it if needed.