Refactor embed.js.erb #5550
Refactor embed.js.erb #5550
Conversation
| var parts = pairs[i].split("="); | ||
| args[parts[0]] = decodeURIComponent(parts[1] || ""); | ||
| } | ||
| var args = Object.fromEntries(new URLSearchParams(window.location.search).entries()); |
There was a problem hiding this comment.
Do you actually need .entries() here? I believe the URLSearchParams object itself should be iterable?
| hot: ["HOT"], | ||
| mapnik: ["Mapnik", mapnikOptions] | ||
| }; | ||
| baseLayers["cycle map"] = baseLayers.cyclemap; |
There was a problem hiding this comment.
I think I'd just include this in the object definition after the cyclemap line. I realise it means duplicating the right hand side but I'm not sure that's any worse than this and I think it makes it a bit easier to see what's going on - also baseLayers could then be const I think?
There was a problem hiding this comment.
const defines only the variable. Ii does not prevent changing the properties of the object.
But yes, changing all var into let/const would be good because they work with much less surprise.
I would do this in a separate PR (and probably active lint rule later)
There was a problem hiding this comment.
which was/will be done in #5558
| var baseLayers = { | ||
| cyclosm: ["CyclOSM"], | ||
| cyclemap: ["CycleMap", thunderforestOptions], | ||
| transportmap: ["TransportMap", thunderforestOptions], | ||
| hot: ["HOT"], | ||
| mapnik: ["Mapnik", mapnikOptions] | ||
| }; | ||
| baseLayers["cycle map"] = baseLayers.cyclemap; | ||
| var [layer, options] = baseLayers[args.layer] || baseLayers.mapnik; | ||
| new L.OSM[layer](options || {}).addTo(map); |
There was a problem hiding this comment.
Don't we have layers.yml with all layers config? Only mapnik needs a special case, the rest can be generated from there. Maybe "cycle map" is also a special case, or we can remove spaces before looking up the layer.
There was a problem hiding this comment.
I also think big potential here was hidden in the else-if chain. I definitely can see the next version here be layers.yml-based, but it should be filtered to what's necessary and include toggling to leafletOsmDarkId.
And I haven't figured out how to do that compactly so this is more of a stop-gap that I can throw a ternary onto to add dark mode later.
Refactors from #5519 and #5522 without adding dark mode (yet).