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

Enable dark theme if user prefers dark color scheme - no javascript version #4761

Merged

Conversation

AntonKhorev
Copy link
Collaborator

@AntonKhorev AntonKhorev commented May 6, 2024

We probably can enable dark mode now.

This version uses system settings with media queries and doesn't need javascript. It is enabled by the Bootstrap option $color-mode-type: media-query. Previous version #4653 used javascript to track system settings and change data-bs-theme attributes accordingly.

The problem with this no-javascript approach is that data-bs-theme attributes are ignored. But we're already using them for inverting close buttons on dark banners. When in light mode, dark banners have their close buttons inverted because banners themselves have a dark theme attribute. Aside from not working with $color-mode-type: media-query, this approach has another drawback: you can't invert buttons on light banners in dark mode. Once the entire webpage gets its dark mode attribute, you can't go back to light mode.

The solution in this PR is:

  • use $color-mode-type: media-query that will take care of most of light/dark mode styling
  • use data-bs-theme on banners
  • add css to enable/disable close button inversion inside elements with data-bs-theme

image
image
image
image

@AntonKhorev AntonKhorev marked this pull request as draft May 9, 2024 14:07
@AntonKhorev
Copy link
Collaborator Author

AntonKhorev commented May 9, 2024

Was conflicting with scale control color fixes, now updated.

@AntonKhorev AntonKhorev force-pushed the dark-mode-with-close-button branch from f53c580 to 5e1093e Compare May 9, 2024 14:16
@AntonKhorev AntonKhorev marked this pull request as ready for review May 9, 2024 14:16
@AntonKhorev AntonKhorev mentioned this pull request Nov 7, 2024
@gravitystorm gravitystorm merged commit ae00fa8 into openstreetmap:master Nov 13, 2024
20 checks passed
@gravitystorm
Copy link
Collaborator

Looks good to me! I think it's worth starting with the automatic mode, since that should just work for most people (and will automatically support day/night switching for devices that do that).

@DaveF63
Copy link

DaveF63 commented Nov 14, 2024

I like dark modes in general, but can someone (other than TH) explain the reason for the lack of choice & the dimmed tile window?

@tomhughes
Copy link
Member

I like dark modes in general, but can someone (other than TH) explain the reason for the lack of choice & the dimmed tile window?

Is my opinion not welcome then? What exactly have I done to upset you?

@Vectorial1024
Copy link

@DaveF63

Random OSM user passing by, but my guess is that, per gravitystorm/openstreetmap-carto#5044, there are actually no "official" dark tiles from osm-carto yet, so the dimming is a stop-gap measure to get dark mode published. I would expect the tile-dimming problem to be dealt with sometime later by someone else.

@openstreetmap openstreetmap locked as resolved and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants