Draft PR for discussion: transition to CSS varaibles only for type #1044
Conversation
|
|
||
| .mzp-u-title-2xl { | ||
| @include text-title-2xl; | ||
| font-family: $title-font-family; |
There was a problem hiding this comment.
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.
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.
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?
There was a problem hiding this comment.
Minor preferences:
- scale/theme prefixes on CSS custom properties look good to me, I like the extra indication of change based on context
var()syntax doesn't bother me but happy to save a few characters with alias to Sass variables
There was a problem hiding this comment.
if we follow the rule that CSS variables should ONLY be used for dynamic variables then maybe the prefix is unnecessary
I'd agree here that prefixes are unnecessary if we only use CSS variables for dynamic values and don't use a sass alias (the var() reads really nicely)
There was a problem hiding this comment.
I guess the issue is it doesn't distinguish between values that are variable based on theme only (i.e. font family will change between themes but be static within that theme) vs. multiple factors (i.e. title size will change based on theme and across screen sizes)
| line-height: var(--title-2xl-line-height); | ||
| } | ||
| } | ||
| font-size: var(--title-2xl-size); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Turns out this is a reserved namespace in Sass.
stephaniehobson
force-pushed
the
css-vars
branch
from
f3f82aa to
1d59bc4
Compare
|
For color naming, I think it's great to have variables that can react to both theme (fx/moz) and color-scheme (light/dark). We'll need to update m24 naming to match Protocol's usage-based names (like 
Does more reliance on CSS properties and inheritance allow us to remove some of the link color mixins? |
|
(Might be out of scope for discussion) Could we replace the hover colors with a function? (Sass darken or CSS color-mix or relative colors) Two fewer tokens to maintain, easier to scale a consistent hover style if we add other link colors later |
janbrasna
left a comment
janbrasna
left a comment
There was a problem hiding this comment.
The failed check is just a linter being loud about whitespace, nothing more.
Couple of naive questions follow, esp. if the change to the px-to-rem mixin is intentional:
| --body-md-size: 0.875rem; | ||
| --body-sm-size: 0.75rem; | ||
| --body-xs-size: 0.688rem; | ||
| // --body-line-height: 1.5; doesn't change, no need to update |
There was a problem hiding this comment.
IIUC it's actually 1.2:
| // --body-line-height: 1.5; doesn't change, no need to update | |
| // --body-line-height: 1.2; doesn't change, no need to update |
not that it matters, just in case it's ever uncommented to not break the scale…
| --body-sm-size: 0.875rem; | ||
| --body-xs-size: 0.75rem; | ||
| --body-line-height: 1.2; | ||
| // --body-line-height: 1.2; doesn't change, no need to update |
There was a problem hiding this comment.
After the updates to L50–L58 also the other line-height title vars here above don't change (from 1.1 to 1.1), not sure how much of it you plan to omit.
This also applies to _firefox.scss theme where it is 1.5 before and also after mq adjustments.
There was a problem hiding this comment.
I've omitted all the unnecessary re-definitions in both Mozilla and Firefox theme files instead of commenting them out. The code is cleaner, hopefully it doesn't confuse anyone.
There was a problem hiding this comment.
Nice catch, thanks ;)
| * Headings are now [balanced](https://developer.mozilla.org/docs/Web/CSS/text-wrap-style#balanced_text), which can impact other wrapping rules. Make sure any changes to `h1`–`h6` rendering end up styled as expected, especially if you apply any `white-space`, `word-break` or `hyphens` customizations. | ||
| * Dropped support for font-sizes in pixels, all font-sizes are in rem now. | ||
| * Removed `text-display-*` mixins. See migration notes for version 11.0.2. | ||
| * See notes for [Protocol Assets 5.4.0](https://github.com/mozilla/protocol-assets/blob/main/CHANGELOG.md#540) |
There was a problem hiding this comment.
FYI in case there are no changelog conflicts, this is only a kind reminder to rebase anyways, to move the entries from already released v20- blocks to unreleased section.
stephaniehobson
changed the title
|
Do Not Merge until Icon release is published |
Draft PR for discussion.