Convert stylesheets to use css custom properties#2755
Conversation
|
Thanks! Yeah, it's a bit big!
…have you seen a path to removing Sass entirely? That'd be fun. |
Cool, I'll work on breaking it up.
I think so. I haven't done a full audit or anything but css has come a long way. I think the blocking factor would be which old browsers does administrate need to support. |
edwardloveall
force-pushed
the
el-css-vars
branch
2 times, most recently
from
543aa98 to
e5a78ea
Compare
edwardloveall
changed the title
edwardloveall
changed the title
|
Alright here's a start with a much smaller scope. I see two ways forward:
Preference? I lean toward the one branch thing since it's less branch juggling for me, but you probably have more to keep track of as maintainer so I think this is up to you. |
|
One branch is better, I think, especially if it's easier for you. |
edwardloveall
force-pushed
the
el-css-vars
branch
from
682b0c6 to
06503e3
Compare
|
|
||
| &:hover { | ||
| color: mix($black, $action-color, 25%); | ||
| color: var(--action-color-active); |
There was a problem hiding this comment.
This changed ever so slightly. There were two selectors that used $action-color as their base. This one mixes in 25% $black and &:not(.link):hover in buttons.scss mixes in 20% $black. I went with the 20% in both places so we only had one "it's like $action-color but tinted" variable because button backgrounds are more prominant than text hovers.
edwardloveall
force-pushed
the
el-css-vars
branch
from
ccd3c7c to
229cf20
Compare
There was a problem hiding this comment.
As noted in the commit message, this is quite a bit bigger than it's scss counterpart. I was seeing if I could really remove all sass variables, and I did! Also, in about a year it's probably pretty safe to use color-mix and this gets way smaller (as seen in the comment at the start of the file).
|
Alright, this is ready. I tried to call out a couple places that are a bit messy or surprising. However, this replaces all sass variables with css variables. It's slightly more verbose because sass can still do more than css at this point, but give it about a year and (also, updated the PR description) |
|
Nice, this looks great! I'm going to sit on it for a little bit, as I work out where it fits inside the release. What else would we want to do for removing Sass? |
|
Of the top of my head, assuming you're threshold for browser support is baseline widely available:
|
|
Is this on track with version 1.0.0 release? |
|
Can we merge this? :) |
|
It looks like we've got some conflicts to fix, which @edwardloveall might be best placed to do. Then as long as we check off the last few things on the list above, we'll be in a good place to merge it. |
edwardloveall
force-pushed
the
el-css-vars
branch
from
229cf20 to
49e54b8
Compare
|
Rebased.
What do you think about merging this as-is? The list above was for completely removing sass. I could take it on in this PR, but there's a lot here already and all the changes are similar to each other. |
|
Thank you! Oh! Right, yes. I misread. Excellent. Yeah, we shouldn't do that in this PR for sure. I'll read over this again and then I think we can merge it. |
|
Dammit, I left this too long and now it's got conflicts. @edwardloveall, could you do the honours? I've also since come across BaseWatch, and I'd made a note to come back to this in particular once I'd figured out a way to track when things are baseline available. I think for this PR we're good to go, but if we want to try out new features we can try out BaseWatch to be alerted when it's safe to be merged. |
I'd like to move away from sass so that we can fully embrace css custom properties and variables. [color-mix](https://developer.mozilla.org/en-US/docs/Web/CSS/color_value /color-mix) and [relative colors](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_colors/Rela tive_colors) are not yet [baseline widely available](https://developer.mozilla.org/en-US/docs/Glossary/Baseline/Co mpatibility) so I'm deciding to inline them. This inlines just one: a base border color in its hover state.
sass has a function called [rgb], and so does [css]. If I try to use the css rgb in a sass context, sass tries to use it first. If I'm using the [relative color syntax], sass complains that I'm passing too many arguments. One way around this is to use a css property to define whatever I need and then use that instead. [rgb]: https://sass-lang.com/documentation/modules/#rgb [css]: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb [relative color syntax]: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb#using_relative_colors_with_rgb
We want to make way for a grey between grey-0 and the old grey-1
edwardloveall
force-pushed
the
el-css-vars
branch
from
49e54b8 to
d94ecf2
Compare
|
Sure thing @nickcharlton. Hopefully I got it to you in time for this week. If not, let me know if you want a reminder at some point. |
|
The failing tests don't look related, but I'm not too familiar with that part of the code. Something about using a Fiber for i18n in the example app? |
|
Thanks! You did! Mmm, I don't think they're related enough to worry about it. I'm going to merge this so it's (finally) done. Thanks for your hard work on this. |
3 participants
Supporting #2732
This converts all sass color variables (and a couple other variables) to use native css variables instead.
There's a lot here! If you'd prefer, I can figure out how to break this up into smaller parts; maybe one per var or something. It also doesn't get rid of all sass variables. I could go even farther and do that if we want. Just let me know.Almost all variables are split into their own commit for easier review.
I also made sure you can still override variables and I'm happy to report that I followed the existing steps and it worked great.
A related consideration is I know administrate is in its 1.0-beta phase. Is this a breaking change worth making this late in the process? It will surely break current sass variable overrides as those no longer exist to be overridden.
Finally, this is also a very large step in the direction away from sass. While there would still be some work to do to fully remove sass, it's a lot more possible if you'd want to do that (in a separate PR).