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

Introduce Dark Mode (WIP) #3028

Closed

Conversation

migurski
Copy link
Contributor

@migurski migurski commented Jan 1, 2021

This PR offers an alternative approach to browser dark mode, noted by @fishcharlie in #2332 and initially implemented by @bezdna in #2532. Before I spend more than a few hours on this, I’m seeking feedback on an approach that accommodates @gravitystorm’s notes on the prior PR:

… having lots of overrides and !important flags and so on is fine when a user style is the only available option. But that's not a good approach when we have full control over the CSS sent to the user. … To implement a dark mode, I would ideally like to see out-of-the-box support from bootstrap itself. If that's not available, then maybe 10-20 lines of code to set some colour variables, but little more than that. – #2532 (comment)

The alternative implementation here defines *-color and *-invcolor parameters as suggested though they’re somewhat duplicative because common.scss had a large number of bare color literals. I migrated all of these to the parameters file without attempting to interpret their meaning. To support dynamic color mode switching these are referenced in media queries near the default definition of each class. Example:

body {
  font-family: 'Helvetica Neue',Arial,sans-serif;
  font-size: $typeheight;
  line-height: 1.6666;
  color: $common-body-color;
  background-color: $common-body-background-color;
  margin: 0px;
  padding: 0px;
  text-align: left;
  height: 100%;
}

@media (prefers-color-scheme:dark)
{
  body {
    color: $common-body-invcolor;
    background-color: $common-body-background-invcolor;
  }
}

I’m also interested in help from anyone who’d like to collaborate on this PR to make it suitable for use! Some sample items left to be done are:

  • Invert or adjust map colors per @pkrasicki’s suggestion – Dark Mode #2332 (comment)
  • Figure out why .dropdown-menu is not yet working
  • Decide on visual approach for map toolbar
  • Update custom CSS on pages like /help and /about

Here’s a short video showing how this implementation of dark mode behaves.

darkmode.mov

Used Chroma-js with simple inverted luminance:

    function inv(s) { var c1 = Chroma(s); c2 = c1.set('lch.l', 100 - c1.get('lch.l')); return c2 }
Used Chroma-js with adjusted inverted luminance:

    function inv(str)
    {
      var c = Chroma(str);
      // sqrt bends the luminance curve to improve light-on-dark contrast
      c2 = c.set('lch.l', 100 * Math.sqrt((100 - c.get('lch.l')) / 100));
      return c2
    }
@migurski migurski force-pushed the migurski/prepare-for-dark-mode branch from 96306f8 to 17874fd Compare January 1, 2021 23:24
@pkrasicki
Copy link

darkmode
You shouldn't use black color for backgrounds. Use dark gray, it will be easier to read the page.

You will need to make those green buttons lighter and link color (both purple and blue) is too dark as well.

Links in top right need to be a little lighter too. Perhaps use white color for current page.

@pkrasicki
Copy link

pkrasicki commented Jan 2, 2021

Instead of creating additional variables and adding invcolor to their names I think you should just override default variables in @media(prefers-color-scheme: dark), if that's possible to do in SASS.

@migurski
Copy link
Contributor Author

migurski commented Jan 2, 2021

That was the first thing I tried, @pkrasicki! It doesn’t work because the variable definitions are interpreted at compile time while the media queries need to happen at render time. There’s a different potential approach using CSS variables which may interfere with the 5% of users who can’t see them; the @media query has less support but it also defaults to existing behavior.

I’ll apply your suggestion of lightening everything slightly.

@gravitystorm
Copy link
Collaborator

When I start to review this and see "Large diffs are not rendered by default" and parameters.css getting to 150 lines, I don't think that fits my definition of "maybe 10-20 lines of code to set some colour variables, but little more than that." 😄

This approach has a couple of major drawbacks. The first is that you have changed the name of colour variables like $blue to $blue-color, but bootstrap uses some of these colour variables too! This is how the primary button colour on the bootstrap forms matches our house blue.

The second is that you are having to invent an entire colour scheme from scratch, as can be seen with the talk about black backgrounds and coloured buttons. I'd prefer to offload those design choices elsewhere, if possible. For example, to an existing "bootstrap-dark" colour scheme where someone else has evaluated what the primary button colours should be, or just how different should the greys used in the background vs btn btn-light in order to meet accessibility guidelines. I'm really not keen on us making that up from scratch, but if it needs to be done then it needs to be done for all of the colours used in bootstrap. Have a look at https://github.com/twbs/bootstrap-rubygem/blob/4.5-stable/assets/stylesheets/bootstrap/_variables.scss to see what variables are available.

On a more minor point, I don't like the 'inv' naming since they aren't inverted - I don't think we'll be highlighting danger messages in cyan, for example!

You are right to pick out that there are many colours defined in custom.css that need addressing. However, I'd prefer to see them sorted out first in one (or more) separate PRs. For example, by using the bootstrap variables instead of specific RGB values, or even better by refactoring the code to use bootstrap instead of having any custom CSS in the first place. This will at least bring some structure, and reduce the maintenance burden for supporting dark mode later on.

As for the exact mechanism for how to support dark mode - I can't assess if this is the best option. But if you need to define @media (prefers-color-scheme:dark) over and over again, then I don't think it's maintainable. I also don't see how this changes the colour of anything (like form buttons or dropdown menus) where the colours are defined by bootstrap.

Refs twbs/bootstrap#27514

@gravitystorm gravitystorm added the changes requested Changes are needed before the PR will be merged or reviewed again label Jan 6, 2021
@migurski
Copy link
Contributor Author

migurski commented Jan 6, 2021

Okay, thanks Andy! I don't think there’s a way to implement dark mode in a way that respects browser settings without a minimal amount of @media querying, since it’s a client-side consideration and not one that Bootstrap will currently support. I’m closing this WIP, and I appreciate your quick feedback.

@migurski migurski closed this Jan 6, 2021
@pkrasicki
Copy link

Maybe detecting system theme is not the right way then? Instead you could have a button to toggle between default and dark mode. It would require a little bit of JavaScript, but you need it anyway to change map colors in Leaflet.

Since Bootstrap doesn't have a dark theme, is it possible to not use custom CSS?

@tomhughes
Copy link
Member

We are not going to have a button to select dark mode... That's just insane.

@bhousel
Copy link
Member

bhousel commented Jan 7, 2021

For anyone curious, here's how I implemented the insane dark mode slider on https://nsi.guide
osmlab/name-suggestion-index@2f68fe0

It's really just a set of css rules that take effect if .dark class is added to the document root.
You don't need to add all the media queries to the .css because JavaScript has a window.matchMedia function that can give you that answer.

This implementation respects the user's dark mode preference the first time, then uses whatever is stored in local storage every time after that.

@pkrasicki
Copy link

@bhousel I did the same in https://issviewer.com.
dark-theme-toggle

Seems like a reasonable alternative to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes are needed before the PR will be merged or reviewed again
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants