Use map tiles dark mode from leaflet-osm plugin #5426
Conversation
|
I may have gotten a bit overexcited by #5421. |
|
Since the line with the not (yet) supported optional chaining is only to make the updates work without the UI panes (like in the tests) anyway, the complexity of that predicate can be reduced. |
| @@ -31,6 +31,9 @@ L.OSM.layers = function (options) { | |||
| var miniMap = L.map(mapContainer[0], { attributionControl: false, zoomControl: false, keyboard: false }) | |||
| .addLayer(new layer.constructor(layer.options)); | |||
|
|
|||
| if (layer.options.schemeClass) miniMap.getPane("tilePane").classList.add(layer.options.schemeClass); | |||
| miniMap.getPane("tilePane").style.setProperty("--dark-mode-map-filter", layer.options.filter || "none"); | |||
There was a problem hiding this comment.
There is no layer that has a filter option now so this just always removes the filter, plus we don't want to be editing the style anyway as we're trying to get away from using inline styles.
What this should do is change the style rules at
to avoid filtering when the map has thedark class applied.
There was a problem hiding this comment.
If the dark flag/class should have special treatment, should thunderforest-cycle get a special nofilter class as the first layer where the default from the current proposal #5328 (comment) is explicitly asked for (#5395 (comment))? And should further layers also get a class each?
Do you think it is favorable to spread the map layer filter definitions that much into CSS as well? I think having the way the layers from leaflet-osm are used in a single location including filters adds to the maintainability.
There was a problem hiding this comment.
I only mentioned the dark class because you're already applying it, though as far as I know it does nothing currently? I don't care too much about exactly what you call the class but I do want to make the filtering decision dependent on a class rather than on dynamically editing the style.
There was a problem hiding this comment.
Since the opinions in #5328 varied wildly, I wanted to have an easy flag for other devs that an alternate tileset has been loaded, since some might like a different styling like I already mentioned in #5396 (comment).
You are correct in that it doesn't do anything on the unmodified site.
| if (layer.options.schemeClass) container.classList.add(layer.options.schemeClass); | ||
| for (let filterReceiver of [container, document.querySelector(".key-ui")]) { | ||
| if (!filterReceiver) continue; | ||
| filterReceiver.style.setProperty("--dark-mode-map-filter", layer.options.filter || "none"); |
There was a problem hiding this comment.
The same thing applies here - we should be making the style look at the class instead of modifying the style inline in a way that doesn't even work.
I think it's important here to note that we decided in #5328 that only filters approved by the cartographers should be applied. Since currently no cartographers have chosen any filters, then it's expected that any existing filtering (e.g. @hlfan could you also work on the point that @tomhughes raised with regards to setting classes instead of using inline styles? |
|
Yes, however, I can already hear the question "why are you adding a no-dark-filter class to the cyclemap that only sets filter:none instead of inheriting filter:none?" coming up in the new PR, as this doesn't need half of the changes here. |
A version between #5396 and #5397 using the updated version of leaflet-osm without requiring it.
Also removing some ifs that always evaluate true and removed the dependency on the
.key-ui.