Skip to content
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

Add CSS variables for color #38

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

atjn
Copy link
Contributor

@atjn atjn commented Aug 12, 2024

This is a partial implementation of #19 - I decided to split it into separate PR's for each type of variable, since the changes are so massive, and there are unique challenges for each type of variable.

Ps. this requires that you merge spectre-org/spectre-docs#15 because none of this works in Internet Explorer.

This solution is fully backwards compatible, making sure that everything in sass still works as it did before. The addition is that you can now ignore the sass variables and instead change the variables directly in CSS. The native variables are nearly as powerful as the sass variables, with the single downside being that the color of text in buttons will not change between dark and light depending on how dark the background is. That feature is not yet supported in browsers.

There is one detail to be aware of: now that the colors are transformed with CSS rather than Sass functions, the output colors are a tiny bit different. This is apparently because Sass transforms the colors incorrectly and CSS does it correctly. The difference is very hard to notice and I don't think it will be a major problem. But the change should probably be published as semver-major just to be safe.

@sdegrande
Copy link

I would really like to see that PR being merged !

@davestewart
Copy link
Contributor

davestewart commented Sep 9, 2024

Hello, hello... really sorry – I missed this. Taking a look now!

All other PRs merged though, thank you! 🙏

@davestewart
Copy link
Contributor

Hey @atjn,

Thanks; this must have been a huge amount of work, and something that was high-up on the todo list!

I've had a brief look and I see there's newly-created relations between the new _variables-css.scss members and _variables.scss, along with new color functions and mixins.

One thing I like to do in my own projects is store CSS variables as SASS variables, i.e.:

// css
:root { 
  --primary-color: #5755d9 !default;
  ...
}

// scss
$primary-color: var(--primary-color);
...

This way, the SCSS files themselves stay a little simpler:

// using sass variables
.some-class {
  color: $primary-color;
}

// using css variables
.some-class {
  color: var(--primary-color);
}

I see the _variables.scss file is becoming a mix of SASS and CSS variables, which on first sight looks a bit intimidating.

And I haven't yet got round to looking at the gymnastics needed to both silo and manipulate shades, but is this something you had considered?

Perhaps it's not possible or advisable, but my conjecture was it might have made the relationships simpler to grok.

Anyway, as a maintainer, I'm back in action to move the library forward, so let's discuss 👀

@atjn
Copy link
Contributor Author

atjn commented Sep 9, 2024

Hey @davestewart I am very happy to see you active and merging things :)

I've had a brief look and I see there's newly-created relations between the new _variables-css.scss members and _variables.scss, along with new color functions and mixins.

Yes, I have replicated everything that sass does with native css solutions, and have added the -css postfix to show that this is a css solution, rather than a sass solution.

I thought it would be best to just duplicate everything like that to make the changes as small and isolated as possible.

One thing I like to do in my own projects is store CSS variables as SASS variables, i.e.:

I can definitely see the value in that. I think mine and your solution both have pros and cons that are mostly subjective. I always prefer to use the platform when I can, which in this case is CSS variables. There are many advantages to using native solutions instead of custom ones that you are probably already aware of, so I won't spend too much time on that. I would also argue that if you only use sass variables when you are using sass features, you make it easy to distinguish between the two. If you use sass variables as a simplification for css variables, you make it unclear what to expect from each value.

And I haven't yet got round to looking at the gymnastics needed to both silo and manipulate shades, but is this something you had considered?

I am not sure exactly what you are referring to, but I think I have made a pretty good system for changing colors via css functions, that allows for the same level of flexibility that the sass solutions had.

@davestewart
Copy link
Contributor

Cool. Thanks for getting back to me.

Obviously a big PR here, so let me take a look over the next few days (realistically may need to wait for the weekend) then we can move forwards.

Out of interest, what is your gut feeling on the workload for the remainder of the variables?

@atjn
Copy link
Contributor Author

atjn commented Sep 9, 2024

Obviously a big PR here, so let me take a look over the next few days (realistically may need to wait for the weekend) then we can move forwards.

Absolutely, no need to rush. Thanks for taking the time to maintain this!

Out of interest, what is your gut feeling on the workload for the remainder of the variables?

  • Fonts will be a walk in the park
  • Sizing will be even harder than colors because there are so many custom calculations
  • I have no idea about breakpoints, it could be really easy, it could be impossible
  • Z-index will be a walk in the park

I do however think that colors and fonts are by far the thing that most people want to customize, so if we can get those merged, we can relax a little and slowly tackle the remaining variables.

@davestewart
Copy link
Contributor

davestewart commented Sep 9, 2024

I would like to introduce some theming variables at some point too, specifically:

  • padding options: so things like buttons and labels can have a bit more horizontal breathing room (see Review padding and margins across framework #4)
  • border-radius options: so anything with a hard-corner (buttons, labels, cards) can be softened (would like these to be component-appropriate too, so perhaps cards would have different variables to buttons)

We will also need to consider:

  • whether any of the existing PRs will blow up with this PR (may need to merge them first; see Review PRs #5)
  • dark mode (see Dark Mode #18)

@atjn
Copy link
Contributor Author

atjn commented Sep 9, 2024

That sounds like good additions.

As for existing PRs I don't think you need to worry. I have gone out of my way to ensure that this PR changes as little as possible, so there would not be much to change in those other PRs.

I have also thought about dark mode support (and high contrast) while making this, and I believe it would be easy to add on top of this with the use of the dark-light css function.

@davestewart
Copy link
Contributor

OK. Food for thought!

@davestewart
Copy link
Contributor

davestewart commented Sep 11, 2024

Hey @atjn,

Just to keep you updated.

I took a bit out time out earlier to review related projects (and added them to the home page for visibility):

CSS Variables is kinda down the pecking order, and – because of the impact – behind:

I would also like to take a holistic review of all the variable categories before committing to a single one (such as color); as you say spacing could be tricky, so I want to experiment in such a way that any new strategy is consistent.

So I'm gong to devote next weekend to digging into this, and I'm sure much of the work, such as the new functions, will be invaluable.

Either way, we'll get this over the line!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants