@tomhughes commented on this pull request.
This looks pretty good - just a couple of minor suggestions that I've made
inline.
> @@ -15,14 +15,7 @@ I18n.default_locale = <%= I18n.default_locale.to_json %>;
I18n.fallbacks = true;
window.onload = function () {
- var query = (window.location.search || "?").slice(1),
- args = {};
-
- var pairs = query.split("&");
- for (var i = 0; i < pairs.length; i++) {
- var parts = pairs[i].split("=");
- args[parts[0]] = decodeURIComponent(parts[1] || "");
- }
+ var args = Object.fromEntries(new
URLSearchParams(window.location.search).entries());
Do you actually need `.entries()` here? I believe the `URLSearchParams` object
itself should be iterable?
> - new L.OSM.CycleMap(thunderforestOptions).addTo(map);
- } else if (args.layer === "transportmap") {
- new L.OSM.TransportMap(thunderforestOptions).addTo(map);
- } else if (args.layer === "hot") {
- new L.OSM.HOT().addTo(map);
- } else {
- new L.OSM.Mapnik(mapnikOptions).addTo(map);
- }
+ var baseLayers = {
+ cyclosm: ["CyclOSM"],
+ cyclemap: ["CycleMap", thunderforestOptions],
+ transportmap: ["TransportMap", thunderforestOptions],
+ hot: ["HOT"],
+ mapnik: ["Mapnik", mapnikOptions]
+ };
+ baseLayers["cycle map"] = baseLayers.cyclemap;
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?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5550#pullrequestreview-2574202826
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/5550/review/2574202...@github.com>
_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev