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

CSS Variables #1409

Open
wants to merge 4 commits into
base: refactor/rxjs-typescript
from

Conversation

@Stanzilla
Copy link
Contributor

Stanzilla commented Dec 25, 2019

This is probably how I would start the conversion, do note that I added a dark-mode root element already but it's unused so far.

@Stanzilla Stanzilla changed the base branch from master to refactor/rxjs-typescript Dec 25, 2019
@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Dec 28, 2019

Thanks for your work! However, we definitely need to move dark mode and theme switching out of this PR, as I've prioritized them post-v5 (see #1306). The main reason is that I haven't decided where to put the switch and what the actual semantics should be, e.g. light mode, dark mode, automatic mode (based on preference), etc.

@Stanzilla

This comment has been minimized.

Copy link
Contributor Author

Stanzilla commented Dec 28, 2019

Will do

@Stanzilla Stanzilla force-pushed the Stanzilla:css-variables branch from 22dac89 to 1a8dde4 Dec 28, 2019
@Stanzilla Stanzilla changed the base branch from refactor/rxjs-typescript to master Dec 28, 2019
@Stanzilla Stanzilla changed the base branch from master to refactor/rxjs-typescript Dec 28, 2019
@Stanzilla

This comment has been minimized.

Copy link
Contributor Author

Stanzilla commented Jan 4, 2020

Is the current state what you had in mind or want something else?

@squidfunk

This comment has been minimized.

Copy link
Owner

squidfunk commented Jan 4, 2020

I'm still on vacation, haven't had the time to check it out. I think I'll come back with some feedback in the middle of January.

Copy link
Owner

squidfunk left a comment

Quickly skimmed through your changes, looks good overall. However, we should switch to HSL to allow interpolation using calc.

@@ -128,7 +128,7 @@ button[data-md-color-accent] {

// Repository containing source
.md-nav__source {
background-color: mix($color, $md-color-black, 75%);
background-color: $color; // TODO: https://codyhouse.co/blog/post/how-to-combine-sass-color-functions-and-css-variables mix($color, var(--md-color-text), 75%);

This comment has been minimized.

Copy link
@squidfunk

squidfunk Jan 4, 2020

Owner

We should use hsl as a color space, because then we can easily mix colors and deduce shades.

This comment has been minimized.

Copy link
@Stanzilla

Stanzilla Jan 4, 2020

Author Contributor

Yup, that's what the link is about, just not sure which parts we should implement as custom functions yet.

This comment has been minimized.

Copy link
@squidfunk

squidfunk Jan 5, 2020

Owner

I read the article and must say, that it sounds very complicated for what we're trying to achieve and it doesn't solve all of our problems, namely that users should be able to override colors with their own CSS variables. IMHO the UX should be:

  1. Set one of the default Material colors via mkdocs.yml - this should be possible as of now
  2. Override colors via CSS variables on the :root pseudo class or any element via extra CSS

We cannot use any SASS functions or mixins implementing this approach, because users should be able to override colors without compiling the whole theme again. This would mean that users would be obliged to define the H, S and L parts of the color separately, so all shades can be deduced. I guess that's too complicated and will lead to a lot of issues, so we should maybe (at the current moment) stick to RGB. The property definitions using the mix function should be normalized as separate CSS variables.

@@ -128,7 +128,7 @@ button[data-md-color-accent] {

// Repository containing source
.md-nav__source {
background-color: mix($color, $md-color-black, 75%);
background-color: $color; // TODO: https://codyhouse.co/blog/post/how-to-combine-sass-color-functions-and-css-variables mix($color, var(--md-color-text), 75%);

This comment has been minimized.

Copy link
@squidfunk

squidfunk Jan 5, 2020

Owner

I read the article and must say, that it sounds very complicated for what we're trying to achieve and it doesn't solve all of our problems, namely that users should be able to override colors with their own CSS variables. IMHO the UX should be:

  1. Set one of the default Material colors via mkdocs.yml - this should be possible as of now
  2. Override colors via CSS variables on the :root pseudo class or any element via extra CSS

We cannot use any SASS functions or mixins implementing this approach, because users should be able to override colors without compiling the whole theme again. This would mean that users would be obliged to define the H, S and L parts of the color separately, so all shades can be deduced. I guess that's too complicated and will lead to a lot of issues, so we should maybe (at the current moment) stick to RGB. The property definitions using the mix function should be normalized as separate CSS variables.

:root {
// Primary and accent colors
--md-color-primary: #{$md-color-primary};
--md-color-primary-values: #{hex2rgb($md-color-primary)};

This comment has been minimized.

Copy link
@squidfunk

squidfunk Jan 5, 2020

Owner

This looks like a smell to me. Where do we need those values? For different opacities? We should define those color-opacity combinations as variables.

This comment has been minimized.

This comment has been minimized.

Copy link
@squidfunk

squidfunk Jan 6, 2020

Owner

I'll check out your changes when I'm back and will take care of merging. Thanks again for your support!

This comment has been minimized.

Copy link
@squidfunk

squidfunk Jan 25, 2020

Owner

I will proceed on this PR when I finished all the JavaScript migration. Sorry for the delay!

Stanzilla added 4 commits Dec 25, 2019
@Stanzilla Stanzilla force-pushed the Stanzilla:css-variables branch from 1a8dde4 to 70091e5 Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.