Fluid type scale #216
Fluid type scale #216
Conversation
|
Thank you so much for all your work on this, @codeAbul ! 🎉 |
|
Closes #47. |
|
Thank you @tbrasington for the updates to line-height and refining scales! |
|
@ovlb Feel free to approve this if you and @tbrasington are good with it! |
|
@codeAbul FYI: I already had a look and overall it looks fine. I’ll write a review that’s worthy of your effort tomorrow, promised! |
|
The foundation is solid and in a good place to be extended it as the new features get built out. As a side note, what may be needed next is guides on pairings of steps and styles. |
| @@ -1,7 +1,10 @@ | |||
| @import 'typography'; | |||
There was a problem hiding this comment.
We import this in css/base.scss, I think you were adding this here because otherwise the SASS variables you are defining in the typography partial would not be accessible?
Two ways to mitigate this, both cleaning up the structure a bit:
- Define the SASS variables in
abstracts/variables(or inbase/_fontswhere the font weights and families are defined) - Change the SASS variables to custom properties and define them in
base/custom-properties
| body { | ||
| border-top: 1rem solid $primary-color; | ||
| font-family: $sans-serif; | ||
| font-size: pxToRem(20); | ||
| font-size: var(--step0); | ||
| line-height: $line-height-heading; |
There was a problem hiding this comment.
Why are you adding $line-height-heading to the body element? I see that you are overwriting this in multiple places down below. I would propose adding $line-height-body here (because it’s the body) and delete all places were you are setting $line-height-body on other elements. Headlines etc. can still have the deviating line-height. This way we let the cascade of CSS work for us.
| line-height: $line-height-heading; | |
| line-height: $line-height-body; |
| // number of positive steps (--step1, --step2 etc) | ||
| // and number of negative steps (--step-1, --step-2 etc) |
There was a problem hiding this comment.
These names are not really clear. Hyphenating is common to separate words, especially in CSS where we don’t use camel case. Can you change these two be either --step-pos-1 and --step-neg-1 or for positive use --step-1 (as this is what we mostly use) and only namespace the negative variables, e.g. --step-minus-1
There was a problem hiding this comment.
I agree. Will update
| letter-spacing: 0.1; | ||
| margin: .75rem 0; | ||
| font-family: $ext-sans; | ||
| font-size: var(--step0); |
There was a problem hiding this comment.
I believe step-0 is not the correct value here, as .sub-headline should be larger then the body text.
| @@ -31,17 +31,18 @@ | |||
|
|
|||
| & > p { | |||
| font-family: $sans-serif; | |||
| font-size: 1.5rem; | |||
| font-size: var(--step0); | |||
There was a problem hiding this comment.
I think you changed this here because 1.5rem made the first paragraph in definition mostly unreadable on mobile?
|
@ovlb Thank you for the reviews. I shall address them this weekend. Sorry, been busy with stuff! Thanks again |
|
@codeAbul Your pace is the right pace :) Looking forward to your updates! |
|
@codeAbul Checking in to see if you're still interested and able to close out this PR. As far as I can tell we're very close! |
|
Hi @tatianamac i am so sorry I have left this out in the wind , thanks to the mess of pandemic and job change. I will work on this tonight. Again sorry for the delay. Hope you and yours are keeping well, as well as one can be in these times. |
|
@codeAbul : Hello! A no-pressure check to see how you're feeling about this PR (I'm going through all issues). Are you still interested in finishing it out? (It might very well be finished; it's hard for me to tell for certain). |
Further improvements possible: