[openstreetmap/openstreetmap-website] New note page on low zooms (PR #5443)

[Tom Hughes]

@tomhughes commented on this pull request.



> @@ -35,11 +35,13 @@ OSM.NewNote = function (map) {
     OSM.router.route("/note/new");
   });
 
-  function createNote(marker, form, url) {
-    var location = marker.getLatLng().wrap();
+  function createNote(form, url) {
+    if (!newNoteMarker) return;

Why do we now need to check this? As far as I can see nothing has changed and if can be null now then it always could be?

> -
-      halo = L.circleMarker(loc, {
-        weight: 2.5,
-        radius: 20,
-        fillOpacity: 0.5,
-        color: "#FF6200"
-      });
+  function addHalo(latlng) {
+    if (halo) map.removeLayer(halo);
+    halo = L.circleMarker(latlng, {
+      weight: 2.5,
+      radius: 20,
+      fillOpacity: 0.5,
+      color: "#FF6200"
+    });
+    map.addLayer(halo);

The vertical whitespace was there to make this more readable...

> @@ -130,11 +127,14 @@ OSM.NewNote = function (map) {
     });
 
     newNoteMarker.on("dragstart dragend", function (a) {
-      newHalo(newNoteMarker.getLatLng(), a.type);
+      removeHalo();
+      if (a.type !== "dragstart") {

Why not check for the type being `dragend` which would be much easier to reason about than having to find the set of captured events and then remove `dragstart` from that to work out when this is going to trigger?

> @@ -24,4 +24,25 @@ class CreateNoteTest < ApplicationSystemTestCase
       end
     end
   end
+
+  test "can open new note page when zoomed out" do

Wouldn't it make more sense to start zoomed in and then test that things disable when you zoom out? It's more realistic than starting from a zoomed out URL which is something the site would never offer as the control is still disabled at low zoom.

I mean there's nothing wrong with testing this, but we should test that zooming out triggers disablement as well.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5443#pullrequestreview-2525759939
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/5443/review/2525759939 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20241230/b47f5325/attachment.htm>