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

Dark theme #1177

Closed
wants to merge 1 commit into from
Closed

Dark theme #1177

wants to merge 1 commit into from

Conversation

yacinehmito
Copy link

@yacinehmito yacinehmito commented Jan 25, 2017

This commit makes changing to a dark theme much easier.

It introduces a lot of changes to the overall organisation of the css variables and how they are used in the various theme.css files. I've done my best to make the visual changes to the default theme as minimal as possible, but I prioritized consistency nonetheless. More on this below.

How-to switch to the dark theme

User

Configure Postcss so it uses the following variables:

{
  'color-background': 'var(--theme-dark-background)';
  'color-background-raised': 'var(--theme-dark-background-raised)';
  'color-background-secondary': 'var(--theme-dark-background-secondary)';
  'color-background-hover': 'var(--theme-dark-background-hover)';
  'color-background-statusbar': 'var(--theme-dark-background-statusbar)';
  'color-background-snackbar': 'var(--theme-dark-background-snackbar)';
  'color-text': 'var(--theme-dark-text)';
  'color-text-secondary': 'var(--theme-dark-text-secondary)';
  'color-text-disabled': 'var(--theme-dark-text-disabled)';
  'color-text-snackbar': 'var(--theme-dark-text-snackbar)';
  'color-faded': 'var(--theme-dark-faded)';
  'color-divider': 'var(--theme-dark-divider)';
  'color-switch-thumb-off': 'var(--theme-dark-switch-thumb-off)';
  'color-switch-track-off': 'var(--theme-dark-switch-track-off)';
  'color-switch-disabled-thumb': 'var(--theme-dark-switch-disabled-thumb)';
  'color-switch-disabled-track': 'var(--theme-dark-switch-disabled-track)';
}

Developer

Go into ./components/variables.css and change every instance of theme-light to theme-dark

Rationale

The original goal was simply to use a dark theme in my project. However, it required to dig into the material design guidelines and change many css variables accordingly. Even then, some colors are hardcoded into the theme.css files, so you need to revert to inline styles or custom theming.

Given that the specs themselves mention the dark theme, I thought appropriate to make the changes here.

Process

The process has been the following:

  • Read the guidelines and create a css variable for each different color
    used in ./components/colors.css; these are prefixed by theme-light
    or theme-dark
  • For each theme-dependent variable, add a corresponding variable in
    ./components/variables.css to add a level of indirection
  • Replace every hard-coded color in each theme.css file by a new or existing
    color in the appropriate config.css file
  • Replace every hard-coded color in each config.css file by a color from
    ./components/variable.css (or a function of such a color)

Along the way I tried to fix anything that didn't fit the material design specs.
I also tried to introduce the minimum amount of variables in ./components/variable.css by reusing them or computing a new color based on those.

Benefits

No hard-coded colors

Except in special circumstances, the properties in the theme.css files don't use hard-coded colors. It makes the colors fully configurable from css variables.

Two dependent layers

We have two layer of style configuration through css variables: the global layer (through components/variable.css) and the component layer (the config.css files). Because the component layer entirely depends on the global layer, you can make very impactful color changes with very few definitions.

The same relationship holds between components/colors.css and components/variables.css, where the former defines variables according to the spec and the latter makes use of those.

Dark theme

Because the colors of each theme are in ./components/colors.css alongside the palette, users don't have to refer to the spec in order to use the dark theme.

Pitfalls

Too many global variables

The specs didn't make reusing colors easy.
The statusbar, snackbar and switch button have arbitrary colors that need their own theme-dependent variables, even though their scope is limited. I leaned toward consistency and defined those variables in ./components/colors.css. I can move those to the config.css file of the appropriate component upon request.

Wild guesses

The specs are incomplete so I had to make some colors up.
For the light theme I've been mostly annoyed by the absence of the Chip colors.
For the dark theme, many colors are missing. It impacts the components Table, Chip, Snackbar and others.

Only static

You can only switch to the dark theme statically.
With how things are laid out, I don't think that we can currently reproduce the on-the-fly theme change feature of Material-UI.

Inverted components become problematic

Inverted components are easy to deal with when you assume a light theme.
However, when the theme can be arbitrary, it's hard to come up with a decent result.

Possible solutions were:

  1. Getting rid of it
  2. Try our best with the colors at our disposal and let the user
    override the variables
  3. Introduce global variables for inverted components

I favored 2. The problem is that inverted buttons for the dark theme are not conformant to spec.
The third solution is tempting but it would make the already crowded list of global variables even bigger.
I really think we should reevaluate this feature, as I don't really understand its usefulness.

Detailed changes

Code

  • Every styled component has a config.css, for consistency
  • Renamed and added some variable names in various .config.css for
    consistency
  • Reordered properties in the various config.css so the colors
    always come first (that was for convenience)
  • Replaced hard-coded colors in the various theme.css by a variable from
    the corresponding config.css
  • Made some minor design modifications to the spec page so it stays
    beautiful under the dark theme

Aesthetic

  • Layout now applies the text color and background color given by the corresponding css variables; Text is lighter, background is darker
  • Raised buttons and floating buttons now have a shadow conformant to the material design guidelines; I don't see the difference when rested
  • Changed the border color of the Sidebar and Sidebar-like components to reuse variables;
    I don't see the difference
  • Changed opacity of the flat button's background on hover according to the material design guidelines; It's slightly lighter
  • Cards, Dropdown, Menu, Dialog and Dialog-like components now have the appropriate background color according to the material design guidelines; It's the same
  • Table dividers now have the same color as the other dividers; it's darker and ticker
  • Following the material design guidelines, Drawer now reuses variables that are blacks or different opacity instead of hard-coded greys
  • Probably a bunch of others that I forgot

All these visual changes can be reverted back upon request

Compare in the browser

You can see the result directly following those links:

@javivelasco
Copy link
Member

This is so so so great and very well documented! I need to dig deeper on it and check it out locally properly but it looks very good. It's going to be also very helpful for the migration we are planning where theming variables should be very well defined and documented.

I'm keeping open meanwhile I can check it out but it's a great work, thank you!

@javivelasco
Copy link
Member

Also sorry, I've introduced some conflicts with some codestyle changes pushed recently, may you solve them?

The organisation of the css variables as been reviewed
to accomodate a dark theme.
@yacinehmito
Copy link
Author

Thank you for your appreciation.
I've pushed the requested changed.

@olegstepura
Copy link
Contributor

Hi!
Good job @gpyh! Special thanks for publicly available example!
I noticed some inconsistencies with dark theme in this example:

  • Icon color in AppBar in dark theme should be white (but it may be an issue with AppBar itself, I already experienced issue with placing elements to AppBar in 2.0@beta and text was dark, not white).
  • Also selected checkbox blue color, active switch blue color seem to be too dark for the dark theme (Should be green?).
  • Link color is almost invisible (link is in right bar)
    2017-01-30 12_28_58-react toolbox - spec

@yacinehmito
Copy link
Author

Icon color in AppBar in dark theme should be white

Nice catch! I'll fix this.

Also selected checkbox blue color, active switch blue color seem to be too dark for the dark theme (Should be green?).

It's because I kept the primary color untouched. If a user wants to use the dark theme, a primary color matching it well is to be picked. Here the blue is indeed too dark.

I don't know if the green chosen in the material guidelines is a recommended color or if it happens to be the primary (or accent) color in the example. As this is left to interpretation, I can do whichever you choose.

Link color is almost invisible (link is in right bar)

Same as above. I'm really curious how prevalent are the flat colored buttons on Android though. AFAIK in most cases it's either flat and neutral or raised and colored.

@yacinehmito
Copy link
Author

Icon color in AppBar in dark theme should be white

I've worked on it a bit and it turns out that this is actually very tricky.
Because they are buttons, they are affected by button styles. Those are specific enough that overriding them gets complicated. There's a class for them in appbar/theme.css but properties get erased when computed. I see four possible solutions:

  1. Making the button styles simpler; this would be ideal, but it's likely to be impossible
  2. Making the appbar button styles needlessly more specific; this is ugly
  3. Using the important keyword; this is ugly
  4. Not using buttons; a big change that goes against code reuse

I'm kinda stuck on this problem as none of the outcomes seem to be both possible (AFAIK) and desirable.

@yacinehmito
Copy link
Author

It's been a while and I did not come back to this. Sorry 🙏
Closing it.

@yacinehmito yacinehmito closed this Dec 8, 2019
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.

None yet

3 participants