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.

Sign up for GitHub

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

Use class based map tiles dark mode #5505

Closed
wants to merge 3 commits into from
Closed

Use class based map tiles dark mode #5505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d02b35e
Add class based dark map mode logic
hlfan Jan 16, 2025
2932cca
Add thunderforest layer dark mode style preference
hlfan Jan 16, 2025
6dad6c7
Adapt dark mode map filtering
hlfan Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions app/assets/javascripts/leaflet.map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ L.OSM.Map = L.Map.extend({
if (layerDefinition.apiKeyId && !OSM[layerDefinition.apiKeyId]) continue;

let layerConstructor = L.OSM.TileLayer;
const layerOptions = {};
const layerOptions = { className: "" };

for (const [property, value] of Object.entries(layerDefinition)) {
if (property === "credit") {
Expand All @@ -32,6 +32,11 @@ L.OSM.Map = L.Map.extend({
layerOptions.apikey = OSM[value];
} else if (property === "leafletOsmId") {
layerConstructor = L.OSM[value];
} else if (property === "leafletOsmDarkId" && OSM.isDarkMap() && L.OSM[value]) {
layerConstructor = L.OSM[value];
layerOptions.className += " dark";
} else if (property === "filterClass" && OSM.isDarkMap()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something like darkFilterClass would be better as the property name, so that it's clear what the filtering is for.

layerOptions.className += " " + value;
} else {
layerOptions[property] = value;
}
Expand All @@ -52,10 +57,12 @@ L.OSM.Map = L.Map.extend({
code: "G"
});

this.on("layeradd", function (event) {
if (this.baseLayers.indexOf(event.layer) >= 0) {
this.setMaxZoom(event.layer.options.maxZoom);
}
this.on("layeradd", function ({ layer }) {
if (this.baseLayers.indexOf(layer) < 0) return;
this.setMaxZoom(layer.options.maxZoom);
const key = document.querySelector(".key-ui");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We normally use jQuery rather than native methods like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that multiple ways, but jQuery-based variants always errored out, so I just left it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked further into it, this is on me for having minified leaflet overwrite the $ function.
This works:

jQuery(".key-ui").attr("class", "key-ui " + layer.options.className);

if (!key) return;
key.className = "key-ui " + layer.options.className;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and if jQuery is used then this can use addClass and not have to worry about preserving pre-existing classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have to remove the classes from the previously selected layer. And I'd rather do that in a single line without needing a separate function on layerremove.

});

function makeAttribution(credit) {
Expand Down Expand Up @@ -386,6 +393,14 @@ L.extend(L.Icon.Default.prototype, {
}
});

OSM.isDarkMap = function () {
var mapTheme = $("body").attr("data-map-theme");
if (mapTheme) return mapTheme === "dark";
var siteTheme = $("html").attr("data-bs-theme");
if (siteTheme) return siteTheme === "dark";
return window.matchMedia("(prefers-color-scheme: dark)").matches;
};

OSM.getUserIcon = function (url) {
return L.icon({
iconUrl: url || OSM.MARKER_RED,
Expand Down
9 changes: 4 additions & 5 deletions app/assets/stylesheets/common.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

body {
font-size: $typeheight;
--dark-mode-map-filter: brightness(.8);
}

time[title] {
Expand Down Expand Up @@ -516,13 +515,13 @@ body.small-nav {
}

@mixin dark-map-color-scheme {
.leaflet-tile-container,
.leaflet-layer,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change in class here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the need to stop .leaflet-tile from inheriting the filter from .leaflet-tile-container since .leaflet-tile-container doesn't inherit the filter from .leaflet-layer.

.mapkey-table-entry td:first-child > * {
filter: var(--dark-mode-map-filter);
filter: var(--dark-mode-map-filter, none);
}

.leaflet-tile-container .leaflet-tile {
filter: none;
.no-filter, .dark {
--dark-mode-map-filter: none;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all seems very confused - you've removed the default value for the filter variable, and set a default of none when using it, so why do we need to explicitly set it to none here?

}
}

Expand Down
2 changes: 2 additions & 0 deletions config/layers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
apiKeyId: "THUNDERFOREST_KEY"
canEmbed: true
canDownloadImage: true
filterClass: "no-filter"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that the filter class was going to be for turning on filtering, not for turning it off? My understanding from #5426 is that enabling filtering for a layer will only happen on the basis of agreement with the designer of that layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appeal that as long as there isn't documentation on which layers should get which treatment in dark mode, explicitly specifying at least in one file that this layer already has had a review of how it should be displayed in dark mode is critical for maintainability and avoiding unnecessary re-reviews.

credit:
id: "thunderforest_credit"
children:
Expand All @@ -39,6 +40,7 @@
href: "https://www.thunderforest.com/"

- leafletOsmId: "TransportMap"
leafletOsmDarkId: "TransportDarkMap"
code: "T"
layerId: "transportmap"
nameId: "transport_map"
Expand Down
Loading