Use class based map tiles dark mode #5505
Conversation
| } else if (property === "leafletOsmDarkId" && OSM.isDarkMap() && L.OSM[value]) { | ||
| layerConstructor = L.OSM[value]; | ||
| layerOptions.className += " dark"; | ||
| } else if (property === "filterClass" && OSM.isDarkMap()) { |
There was a problem hiding this comment.
I think something like darkFilterClass would be better as the property name, so that it's clear what the filtering is for.
| this.on("layeradd", function ({ layer }) { | ||
| if (this.baseLayers.indexOf(layer) < 0) return; | ||
| this.setMaxZoom(layer.options.maxZoom); | ||
| const key = document.querySelector(".key-ui"); |
There was a problem hiding this comment.
We normally use jQuery rather than native methods like this.
There was a problem hiding this comment.
I tried that multiple ways, but jQuery-based variants always errored out, so I just left it this way.
There was a problem hiding this comment.
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);| this.setMaxZoom(layer.options.maxZoom); | ||
| const key = document.querySelector(".key-ui"); | ||
| if (!key) return; | ||
| key.className = "key-ui " + layer.options.className; |
There was a problem hiding this comment.
...and if jQuery is used then this can use addClass and not have to worry about preserving pre-existing classes.
There was a problem hiding this comment.
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.
| @@ -31,6 +31,7 @@ | |||
| apiKeyId: "THUNDERFOREST_KEY" | |||
| canEmbed: true | |||
| canDownloadImage: true | |||
| filterClass: "no-filter" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| .leaflet-tile-container .leaflet-tile { | ||
| filter: none; | ||
| .no-filter, .dark { | ||
| --dark-mode-map-filter: none; |
There was a problem hiding this comment.
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?
| @@ -516,13 +515,13 @@ body.small-nav { | |||
| } | |||
|
|
|||
| @mixin dark-map-color-scheme { | |||
| .leaflet-tile-container, | |||
| .leaflet-layer, | |||
There was a problem hiding this comment.
Why the change in class here?
There was a problem hiding this comment.
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.
|
Superseded by #5507. |
A cleaner, class-based variant of #5426, enabling moving more logic out of
layeraddintoinitialize.