feat: map measuring tape (#452)#458
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a toggleable measuring tool to the map. A ruler button joins the existing top-left control cluster (zoom/locate/satellite); activating it drops two draggable endpoints (A/B) straddling the viewport center, connected by a dashed line with a live distance label. - Endpoints are anchored to lat/lng, so they stay put through zoom/pan; distance is geodesic (map.distance) and only changes when an end moves. - Label shows imperial primary (ft/mi) with metric secondary (m/km), shrink-wrapped and centered on the line midpoint. - Toggling off removes all layers; toggling on resets to center. Frontend-only (Map.jsx + App.css); no backend or DB changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements a map measuring tape feature that allows users to measure the distance between two draggable endpoints on the map. It adds a toggle button to the map controls, introduces the MeasureTape component to manage the markers, polyline, and distance tooltip, and includes the necessary styling and specification documents. The review feedback highlights two key improvements: refactoring the ZoomLocateControl component's effect hook with a ref to prevent unnecessary re-renders when toggle states change, and disabling click propagation on the measurement handles to prevent unintended map zoom and POI deselection.
| } | ||
| }; | ||
| }, [map, handleLocate, useSatellite, onSatelliteToggle]); | ||
| }, [map, handleLocate, useSatellite, onSatelliteToggle, measureMode, onMeasureToggle]); |
There was a problem hiding this comment.
The ZoomLocateControl is currently recreated and re-added to the map every time measureMode or useSatellite changes because they are included in the useEffect dependency array. This causes the entire control cluster to flash and re-render in the DOM, which can disrupt accessibility (focus states) and degrade performance.
To prevent this, you can store the toggle callbacks in a useRef and keep the control's creation effect stable by only depending on map and handleLocate.
Here is how you can refactor it:
const callbacksRef = useRef({ onSatelliteToggle, onMeasureToggle });
useEffect(() => {
callbacksRef.current = { onSatelliteToggle, onMeasureToggle };
});
useEffect(() => {
// ... inside ZoomLocateControlClass onAdd:
L.DomEvent.on(satelliteToggle, 'click', function(e) {
L.DomEvent.preventDefault(e);
if (callbacksRef.current.onSatelliteToggle) {
callbacksRef.current.onSatelliteToggle();
}
});
L.DomEvent.on(measure, 'click', function(e) {
L.DomEvent.preventDefault(e);
if (callbacksRef.current.onMeasureToggle) {
callbacksRef.current.onMeasureToggle();
}
});
// ...
}, [map, handleLocate]);| const markerA = L.marker(startA, { draggable: true, icon: makeHandle('A'), zIndexOffset: 1200, keyboard: false }).addTo(map); | ||
| const markerB = L.marker(startB, { draggable: true, icon: makeHandle('B'), zIndexOffset: 1200, keyboard: false }).addTo(map); |
There was a problem hiding this comment.
Clicking or double-clicking on the measurement handles (A and B) will propagate to the map. This causes the active POI in the sidebar to be deselected (via the map's click handler) and the map to zoom in (via double-click).
To prevent this, disable click propagation on the marker DOM elements once they are added to the map.
| const markerA = L.marker(startA, { draggable: true, icon: makeHandle('A'), zIndexOffset: 1200, keyboard: false }).addTo(map); | |
| const markerB = L.marker(startB, { draggable: true, icon: makeHandle('B'), zIndexOffset: 1200, keyboard: false }).addTo(map); | |
| const markerA = L.marker(startA, { draggable: true, icon: makeHandle('A'), zIndexOffset: 1200, keyboard: false }).addTo(map); | |
| const markerB = L.marker(startB, { draggable: true, icon: makeHandle('B'), zIndexOffset: 1200, keyboard: false }).addTo(map); | |
| if (markerA._icon) L.DomEvent.disableClickPropagation(markerA._icon); | |
| if (markerB._icon) L.DomEvent.disableClickPropagation(markerB._icon); |
…458 review) The ZoomLocateControl effect listed the satellite/measure state and inline toggle handlers in its deps, so the entire Leaflet control was torn down and recreated on every render/toggle (flicker, wasted DOM work). - Make the toggle handlers stable with useCallback in Map. - Stop reading useSatellite/measureMode inside onAdd; the dedicated sync effects already own each button's active state and run on mount. - Effect now depends only on map + stable callbacks, so the control is built exactly once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop keyboard:false on the endpoint markers so they aren't removed from the tab order; the option wasn't load-bearing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The measure button is the 5th button in the zoom-locate control cluster; update the button-order test to assert its presence and position. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
map.distance) and only changes when an endpoint moves.Frontend-only:
frontend/src/components/Map.jsx+frontend/src/App.css. No backend or DB changes. Spec/plan under.specify/specs/032-measure-tape/.Closes #452
Test plan
./run.sh buildpasses (frontend compiles in-image)./run.sh test, run on deploy)🤖 Generated with Claude Code