-
Notifications
You must be signed in to change notification settings - Fork 79
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
Draft PR for discussion: transition to CSS varaibles only. #1044
base: main
Are you sure you want to change the base?
Conversation
@@ -10,73 +10,41 @@ | |||
.mzp-u-title-2xl { | |||
@include text-title-2xl; | |||
font-family: $title-font-family; |
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.
Maybe font-family should be part of the @include text-title-n
declaration?
--title-xs-line-height: 1.166; | ||
--title-2xs-size: 1.25rem; | ||
--title-2xs-line-height: 1.2; | ||
--title-2xl-size: 3.5rem; |
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.
I've been toying with the idea of variable names that make it clear that they morph with media queries. Alternative name ideas:
--scale-title-2xl-size: 3.5rem;
(colours could usetheme
, nice because it gives a clue of the variable value)--let-title-2xl-size: 3.5rem;
(javascript reference: const vs let, mostly nice because its short, we should not have any const CSS vars, they should be Sass vars)--find-title-2xl-size: 3.5rem;
(nice because all dynamic variables can use the same prefix)
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.
But if we follow the rule that CSS variables should ONLY be used for dynamic variables then maybe the prefix is unnecessary? But then, is there value in making it explicit for contributors and users of the system who don't know the rules?
line-height: var(--title-2xl-line-height); | ||
} | ||
} | ||
font-size: var(--title-2xl-size); |
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.
Would putting the CSS variable into a Sass variable help with readability and authoring speed? Or would it add to confusion?
It would be SO NICE to just type --title-2xl-size
.
f3f82aa
to
1d59bc4
Compare
Draft PR for discussion.