@tomhughes requested changes on this pull request.
> @@ -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()) {
I think something like `darkFilterClass` would be better as the property name,
so that it's clear what the filtering is for.
> @@ -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");
We normally use jQuery rather than native methods like this.
> @@ -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");
+ if (!key) return;
+ key.className = "key-ui " + layer.options.className;
...and if jQuery is used then this can use `addClass` and not have to worry
about preserving pre-existing classes.
> @@ -31,6 +31,7 @@
apiKeyId: "THUNDERFOREST_KEY"
canEmbed: true
canDownloadImage: true
+ filterClass: "no-filter"
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.
> }
- .leaflet-tile-container .leaflet-tile {
- filter: none;
+ .no-filter, .dark {
+ --dark-mode-map-filter: none;
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,
Why the change in class here?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5505#pullrequestreview-2556912892
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/5505/review/2556912...@github.com>_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
