diff --git a/client/src/components/Map/MapView.test.tsx b/client/src/components/Map/MapView.test.tsx index a9e3396b9..b1bc860ea 100644 --- a/client/src/components/Map/MapView.test.tsx +++ b/client/src/components/Map/MapView.test.tsx @@ -40,7 +40,22 @@ vi.mock('react-leaflet', () => ({ {children} ), - Polyline: ({ positions }: any) =>
, + // pathOptions has to reach the DOM: the style props are what the track-colour + // feature (#776) actually asserts on, and the real Leaflet only repaints when + // that object's reference changes. + // bubblingMouseEvents and pane are surfaced too: a jsdom mock cannot reproduce + // Leaflet's event propagation or its pane stacking, so the tests assert the + // options that govern them instead. + Polyline: ({ positions, pathOptions, eventHandlers, bubblingMouseEvents, pane }: any) => ( +
eventHandlers?.click?.()} + /> + ), CircleMarker: () =>
, Circle: () =>
, Tooltip: ({ children }: any) => <>{children}, @@ -154,7 +169,10 @@ describe('MapView', () => { buildMapPlace({ lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]' }), ] render() - expect(screen.getByTestId('polyline')).toBeTruthy() + // Three per track: casing, the visible line, and the invisible fat one + // that catches clicks. The casing is always mounted so that toggling a + // colour never remounts a path into the wrong stacking position. + expect(screen.getAllByTestId('polyline').length).toBe(3) }) it('FE-COMP-MAPVIEW-010: MarkerClusterGroup is rendered', () => { @@ -187,6 +205,67 @@ describe('MapView', () => { expect(screen.queryByTestId('polyline')).toBeNull() }) + // ── Track colours (#776) ────────────────────────────────────────────────── + // The style has to travel through pathOptions: react-leaflet only calls + // setStyle when that object's reference changes, so bare color/weight props + // would freeze at their mount-time value and a recolour would never show. + const trackOptions = () => screen.getAllByTestId('polyline') + .map(el => JSON.parse(el.getAttribute('data-path-options') || 'null')) + .filter(Boolean) + + it('FE-COMP-MAPVIEW-025: a picked route_color beats the category colour', () => { + const places = [buildMapPlace({ + lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]', + category_color: '#00ff00', route_color: '#e11d48', + })] + render() + expect(trackOptions().some(o => o.color === '#e11d48')).toBe(true) + expect(trackOptions().some(o => o.color === '#00ff00')).toBe(false) + }) + + it('FE-COMP-MAPVIEW-026: without route_color the category colour still wins', () => { + const places = [buildMapPlace({ + lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]', + category_color: '#00ff00', route_color: null, + })] + render() + expect(trackOptions().some(o => o.color === '#00ff00')).toBe(true) + }) + + it('FE-COMP-MAPVIEW-027: with neither colour the track keeps the old blue', () => { + const places = [buildMapPlace({ lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]' })] + render() + expect(trackOptions().some(o => o.color === '#3b82f6' && o.weight === 3.5 && o.opacity === 0.75)).toBe(true) + // The casing is mounted but invisible — a track nobody coloured looks + // exactly as it did before. + expect(trackOptions().some(o => o.color === '#ffffff' && o.opacity === 0)).toBe(true) + expect(trackOptions().some(o => o.color === '#ffffff' && o.opacity > 0)).toBe(false) + }) + + it('FE-COMP-MAPVIEW-028: a coloured track gets a white casing underneath', () => { + const places = [buildMapPlace({ + lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]', route_color: '#059669', + })] + render() + expect(trackOptions().some(o => o.color === '#ffffff' && o.weight === 6.5 && o.opacity === 0.7)).toBe(true) + expect(trackOptions().some(o => o.color === '#059669' && o.opacity === 0.9)).toBe(true) + }) + + it('FE-COMP-MAPVIEW-029: clicking a track selects its place', () => { + const onMarkerClick = vi.fn() + const places = [buildMapPlace({ + id: 77, lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]', + })] + render() + const hit = screen.getAllByTestId('polyline') + .find(el => JSON.parse(el.getAttribute('data-path-options') || '{}').weight === 14) + fireEvent.click(hit!) + expect(onMarkerClick).toHaveBeenCalledWith(77) + // Paths bubble to the map by default and the map click clears the selection + // this one just made — the mock cannot reproduce that, so assert the option. + expect(hit!.getAttribute('data-bubbling')).toBe('false') + }) + it('FE-COMP-MAPVIEW-014: marker icon uses base64 image_url for photo places', () => { const dataUrl = 'data:image/jpeg;base64,/9j/4AA' const places = [buildMapPlace({ id: 10, lat: 48.0, lng: 2.0, image_url: dataUrl })] diff --git a/client/src/components/Map/MapView.tsx b/client/src/components/Map/MapView.tsx index 66c3ebc8f..efe257bfc 100644 --- a/client/src/components/Map/MapView.tsx +++ b/client/src/components/Map/MapView.tsx @@ -15,6 +15,7 @@ import { useTransportRoutes } from '../../hooks/useTransportRoutes' import { visibleRouteReservations } from '../../utils/reservationRoutes' import type { Reservation, RouteVia } from '../../types' import { POI_CATEGORY_BY_KEY, type Poi } from './poiCategories' +import { resolveTrackColor, hasManualTrackColor } from './trackColors' import { DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM } from '../../constants/mapDefaults' import { computeMapViewport, TILE_SIZE_RASTER, type ViewportPadding } from '../../utils/mapViewport' @@ -338,6 +339,28 @@ function ZoomTracker({ onZoomStart, onZoomEnd }: { onZoomStart: () => void; onZo return null } +const TRACK_CASING_PANE = 'trek-track-casing' + +/** + * Pane that holds the white casing under GPX tracks (#776). Leaflet stacks paths + * in insertion order, so without a pane of its own a casing mounted later — a + * newly imported track, or one that just got a colour — would paint over an + * earlier track's line. Pane support is an optimization: a renderer without the + * pane API still draws everything, just in insertion order. + */ +function TrackCasingPane({ onReady }: { onReady: (ready: boolean) => void }) { + const map = useMap() + useEffect(() => { + if (typeof map.getPane !== 'function' || typeof map.createPane !== 'function') return + if (!map.getPane(TRACK_CASING_PANE)) { + const pane = map.createPane(TRACK_CASING_PANE) + if (pane) pane.style.zIndex = '398' // under overlayPane (400) and the plugin pane (399) + } + onReady(true) + }, [map, onReady]) + return null +} + function MapClickHandler({ onClick }: MapClickHandlerProps) { const map = useMap() useEffect(() => { @@ -574,6 +597,8 @@ export const MapView = memo(function MapView({ return () => window.removeEventListener('scroll', clear, true) }, [hoveredPlace]) + const [hasCasingPane, setHasCasingPane] = useState(false) + const handleMarkerClick = useCallback((id: number) => { // Clear the hover card right away: the recenter that follows moves the // marker out from under the cursor, so no mouseout will ever fire (#1404). @@ -686,22 +711,66 @@ export const MapView = memo(function MapView({ ) }), [places, selectedPlaceId, dayOrderMap, photoUrls, handleMarkerClick, handleMarkerHover, handleMarkerHoverOut]) - const gpxPolylines = useMemo(() => places.flatMap(place => { + // Parsing track geometry is the expensive part (tracks run to tens of thousands + // of points), so it hangs off `places` alone — a selection change must not + // re-parse every track. + const gpxTracks = useMemo(() => places.flatMap(place => { if (!place.route_geometry) return [] try { const coords = JSON.parse(place.route_geometry) as [number, number][] if (!coords || coords.length < 2) return [] - return [( + return [{ place, coords, cased: hasManualTrackColor(place), color: resolveTrackColor(place) }] + } catch { return [] } + }), [places]) + + // Keeps the click handler out of the polyline memo below: `handleMarkerClick` + // changes on every selection, and depending on it would redraw all tracks. + const markerClickRef = useRef(handleMarkerClick) + markerClickRef.current = handleMarkerClick + + const gpxPolylines = useMemo(() => ( + <> + {/* Casings live in their own pane below the lines. Leaflet stacks paths by + insertion order, so drawing them inline would put the casing of a track + added later — or of one just given a colour — on top of an earlier + track's line. Always rendered, hidden via opacity when there is no + colour, so toggling one never remounts the path. */} + {gpxTracks.map(({ place, coords, cased }) => ( + + ))} + {/* pathOptions, not bare color/weight props: react-leaflet only calls + setStyle when the pathOptions reference changes, so bare props would + stick at their mount-time colour and a repaint would never arrive. */} + {gpxTracks.map(({ place, coords, cased, color }) => ( - )] - } catch { return [] } - }), [places]) + ))} + {/* Invisible fat line on top so the track can actually be hit — 3.5px is + not a target, least of all on touch, and the start markers cluster below + zoom 11. bubblingMouseEvents is essential: paths bubble to the map by + default (markers do not), and the map's own click handler clears the + selection this one just made. */} + {gpxTracks.map(({ place, coords }) => ( + markerClickRef.current(place.id) }} + /> + ))} + + ), [gpxTracks, hasCasingPane]) const TooltipOverlay = !hoverDisabled && hoveredPlace && tooltipPos && !isTouchDevice const CatIcon = TooltipOverlay ? getCategoryIcon(hoveredPlace.category_icon) : null @@ -776,6 +845,7 @@ export const MapView = memo(function MapView({ ] : [])} {/* GPX imported route geometries */} + {gpxPolylines} { expect(glMap.fitBounds.mock.calls.length).toBe(afterDayFit) }) +// ── Track colours (#776) ────────────────────────────────────────────────── + // The gpx layer paints from a per-feature property, so what matters is the + // GeoJSON handed to setData. getSource returns null by default in this suite, + // which makes the effect bail out — the source has to be stubbed per test. + const gpxFeatures = () => { + const gpxSource = { setData: vi.fn() } + glMap.getSource.mockImplementation((id: string) => (id === 'trip-gpx' ? gpxSource : null)) + return () => { + const calls = gpxSource.setData.mock.calls + return calls.length ? (calls[calls.length - 1][0]?.features ?? []) : [] + } + } + + it('FE-COMP-MAPVIEWGL-021: a picked route_color reaches the feature, casing on', async () => { + const features = gpxFeatures() + const places = [buildMapPlace({ + id: 5, lat: 48, lng: 2, route_geometry: '[[48.0,2.0],[49.0,3.0]]', + category_color: '#00ff00', route_color: '#e11d48', + })] + render() + await act(async () => {}) + expect(features()[0]?.properties).toMatchObject({ color: '#e11d48', cased: true, place_id: 5 }) + }) + + it('FE-COMP-MAPVIEWGL-022: without a pick it stays on the category colour and skips the casing', async () => { + const features = gpxFeatures() + const places = [buildMapPlace({ + id: 6, lat: 48, lng: 2, route_geometry: '[[48.0,2.0],[49.0,3.0]]', + category_color: '#00ff00', + })] + render() + await act(async () => {}) + expect(features()[0]?.properties).toMatchObject({ color: '#00ff00', cased: false }) + }) }) diff --git a/client/src/components/Map/MapViewGL.tsx b/client/src/components/Map/MapViewGL.tsx index 784c1d93c..24e853c6a 100644 --- a/client/src/components/Map/MapViewGL.tsx +++ b/client/src/components/Map/MapViewGL.tsx @@ -19,6 +19,7 @@ import LocationButton from './LocationButton' import { useGeolocation } from '../../hooks/useGeolocation' import type { Place, Reservation, RouteVia } from '../../types' import { POI_CATEGORY_BY_KEY, type Poi } from './poiCategories' +import { resolveTrackColor, hasManualTrackColor } from './trackColors' import { buildPoiPopupHtml } from './placePopup' import { pluginsApi, type PluginMapMarker, type PluginMapLayer } from '../../api/client' import { DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM } from '../../constants/mapDefaults' @@ -41,6 +42,7 @@ const PLACE_CLUSTER_SOURCE_ID = 'trip-place-clusters' const PLACE_CLUSTER_CIRCLE_LAYER_ID = 'trip-place-clusters-circle' const PLACE_CLUSTER_COUNT_LAYER_ID = 'trip-place-clusters-count' const PLACE_UNCLUSTERED_LAYER_ID = 'trip-place-unclustered-hit' +const GPX_HIT_LAYER_ID = 'trip-gpx-hit' type PlaceWithCoords = Place & { lat: number; lng: number } @@ -517,6 +519,17 @@ export function MapViewGL({ // gpx geometries source (place.route_geometry) if (!map.getSource('trip-gpx')) { map.addSource('trip-gpx', { type: 'geojson', data: { type: 'FeatureCollection', features: [] } }) + // Casing under the tracks that carry a picked colour (#776) — keeps them + // legible on satellite and dark styles. Untouched tracks are filtered + // out, so they look exactly as they did before. + map.addLayer({ + id: 'trip-gpx-casing', + type: 'line', + source: 'trip-gpx', + filter: ['==', ['get', 'cased'], true], + paint: { 'line-color': '#ffffff', 'line-width': 6.5, 'line-opacity': 0.7 }, + layout: { 'line-cap': 'round', 'line-join': 'round' }, + }) map.addLayer({ id: 'trip-gpx-line', type: 'line', @@ -524,10 +537,45 @@ export function MapViewGL({ paint: { 'line-color': ['coalesce', ['get', 'color'], '#3b82f6'], 'line-width': 3.5, - 'line-opacity': 0.75, + 'line-opacity': ['case', ['==', ['get', 'cased'], true], 0.9, 0.75], }, layout: { 'line-cap': 'round', 'line-join': 'round' }, }) + // Invisible fat line that catches the click — 3.5px is not a target, + // and the start markers cluster below zoom 11, so without this a track + // is unreachable at the very zoom where you compare walks side by side. + map.addLayer({ + id: GPX_HIT_LAYER_ID, + type: 'line', + source: 'trip-gpx', + paint: { 'line-color': '#000', 'line-width': 14, 'line-opacity': 0 }, + layout: { 'line-cap': 'round', 'line-join': 'round' }, + }) + const selectTrack = (e: any) => { // eslint-disable-line @typescript-eslint/no-explicit-any + // A click on a cluster bubble sitting over a track belongs to the + // cluster (zoom-to-expand), not to the line underneath it. + if ( + typeof map.getLayer === 'function' + && map.getLayer(PLACE_CLUSTER_CIRCLE_LAYER_ID) + && typeof map.queryRenderedFeatures === 'function' + && map.queryRenderedFeatures(e.point, { layers: [PLACE_CLUSTER_CIRCLE_LAYER_ID, PLACE_CLUSTER_COUNT_LAYER_ID] }).length > 0 + ) return + const target = e.originalEvent?.target as HTMLElement | undefined + if (target?.closest?.('.mapboxgl-marker, .maplibregl-marker')) return + const placeId = e.features?.[0]?.properties?.place_id + if (typeof placeId === 'number') onClickRefs.current.marker?.(placeId) + } + const setTrackCursor = () => { + const canvas = typeof map.getCanvas === 'function' ? map.getCanvas() : null + if (canvas) canvas.style.cursor = 'pointer' + } + const clearTrackCursor = () => { + const canvas = typeof map.getCanvas === 'function' ? map.getCanvas() : null + if (canvas) canvas.style.cursor = '' + } + map.on('click', GPX_HIT_LAYER_ID, selectTrack) + map.on('mouseenter', GPX_HIT_LAYER_ID, setTrackCursor) + map.on('mouseleave', GPX_HIT_LAYER_ID, clearTrackCursor) } if (!map.getSource(PLACE_CLUSTER_SOURCE_ID)) { map.addSource(PLACE_CLUSTER_SOURCE_ID, { @@ -674,6 +722,14 @@ export function MapViewGL({ && typeof map.queryRenderedFeatures === 'function' && map.queryRenderedFeatures(e.point, { layers: [PLACE_CLUSTER_CIRCLE_LAYER_ID, PLACE_CLUSTER_COUNT_LAYER_ID] }).length > 0 ) return + // Same for a click that landed on a track — it selects the track, it does + // not drop a new place on top of the line. + if ( + typeof map.getLayer === 'function' + && map.getLayer(GPX_HIT_LAYER_ID) + && typeof map.queryRenderedFeatures === 'function' + && map.queryRenderedFeatures(e.point, { layers: [GPX_HIT_LAYER_ID] }).length > 0 + ) return onClickRefs.current.map?.({ latlng: { lat: e.lngLat.lat, lng: e.lngLat.lng } }) }) // Emit the viewport bbox (pan/zoom + once on first idle) so the POI-explore @@ -1155,7 +1211,11 @@ export function MapViewGL({ if (!coords || coords.length < 2) return [] return [{ type: 'Feature' as const, - properties: { color: (place as Place & { category_color?: string }).category_color || '#3b82f6' }, + properties: { + color: resolveTrackColor(place), + cased: hasManualTrackColor(place), + place_id: place.id, + }, geometry: { type: 'LineString' as const, coordinates: coords.map(([lat, lng]) => [lng, lat]) }, }] } catch { return [] } diff --git a/client/src/components/Map/trackColors.ts b/client/src/components/Map/trackColors.ts new file mode 100644 index 000000000..bca982d81 --- /dev/null +++ b/client/src/components/Map/trackColors.ts @@ -0,0 +1,32 @@ +import { TRACK_COLOR_FALLBACK } from '@trek/shared' +import type { Place } from '../../types' + +/** + * The colour a track's polyline is drawn in (#776): a manually picked colour + * wins, then the category colour, then the blue every imported track used to + * get. Both map renderers and the sidebar chip go through here so a track + * never reads as one colour in the list and another on the map. + * + * `category_color` is a SQL alias joined onto the row, not part of the place + * contract, which is why it needs the cast. + */ +export function resolveTrackColor(place: Place): string { + const withCategory = place as Place & { category_color?: string | null } + return place.route_color || withCategory.category_color || TRACK_COLOR_FALLBACK +} + +/** + * What the track would fall back to if its colour were cleared — the category + * colour, or the old blue. Deliberately ignores route_color: this is what the + * picker's auto cell previews, and previewing the current colour there would + * promise the opposite of what clicking it does. + */ +export function inheritedTrackColor(place: Place): string { + const withCategory = place as Place & { category_color?: string | null } + return withCategory.category_color || TRACK_COLOR_FALLBACK +} + +/** True once a track carries a deliberately chosen colour rather than an inherited one. */ +export function hasManualTrackColor(place: Place): boolean { + return Boolean(place.route_color) +} diff --git a/client/tests/helpers/factories.ts b/client/tests/helpers/factories.ts index 58a825118..93bba42c2 100644 --- a/client/tests/helpers/factories.ts +++ b/client/tests/helpers/factories.ts @@ -112,6 +112,7 @@ export function buildPlace(overrides: Partial = {}): Place { google_place_id: null, osm_id: null, route_geometry: null, + route_color: null, place_time: null, end_time: null, duration_minutes: 60,