diff --git a/client/src/pages/sharedTrip/SharedMap.test.tsx b/client/src/pages/sharedTrip/SharedMap.test.tsx new file mode 100644 index 000000000..24e7357cf --- /dev/null +++ b/client/src/pages/sharedTrip/SharedMap.test.tsx @@ -0,0 +1,103 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { render } from '@testing-library/react' +import type { ReactNode } from 'react' +import { computeMapViewport, TILE_SIZE_RASTER } from '../../utils/mapViewport' +import { SharedMap, type MapPlace } from './SharedMap' + +// The camera the map opened with, and every fit made after it. +const mounted: { center: [number, number]; zoom: number }[] = [] +const fitBounds = vi.fn() + +vi.mock('react-leaflet', () => ({ + MapContainer: ({ center, zoom, children }: { center: [number, number]; zoom: number; children: ReactNode }) => { + mounted.push({ center, zoom }) + return
{children}
+ }, + TileLayer: () => null, + Marker: ({ children }: { children?: ReactNode }) =>
{children}
, + Polyline: () => null, + Tooltip: ({ children }: { children?: ReactNode }) =>
{children}
, + useMap: () => ({ fitBounds }), +})) +vi.mock('react-leaflet-cluster', () => ({ + default: ({ children }: { children?: ReactNode }) =>
{children}
, +})) +vi.mock('../../components/Map/VectorBasemap', () => ({ default: () => null })) + +// Synthetic points: a dense cluster plus a spread over a small region, wider than tall. +const places: MapPlace[] = [ + { id: 1, name: 'A', lat: 52.37, lng: 4.89 }, + { id: 2, name: 'B', lat: 52.372, lng: 4.893 }, + { id: 3, name: 'C', lat: 52.33, lng: 5.42 }, + { id: 4, name: 'D', lat: 52.35, lng: 4.3 }, + { id: 5, name: 'E', lat: 52.4, lng: 4.98 }, +] + +const PADDING = { top: 64, right: 48, bottom: 48, left: 48 } + +let cardSize = { width: 0, height: 0 } +const realWidth = Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'clientWidth') +const realHeight = Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'clientHeight') + +function renderMap(list: MapPlace[] = places) { + return render( + {}} />, + ) +} + +describe('SharedMap opening frame (#2549)', () => { + beforeEach(() => { + mounted.length = 0 + fitBounds.mockClear() + // A desktop window far larger than the map card, which is what exposed the bug. + vi.stubGlobal('innerWidth', 1920) + vi.stubGlobal('innerHeight', 1080) + Object.defineProperty(HTMLElement.prototype, 'clientWidth', { configurable: true, get: () => cardSize.width }) + Object.defineProperty(HTMLElement.prototype, 'clientHeight', { configurable: true, get: () => cardSize.height }) + }) + + afterEach(() => { + vi.unstubAllGlobals() + if (realWidth) Object.defineProperty(HTMLElement.prototype, 'clientWidth', realWidth) + if (realHeight) Object.defineProperty(HTMLElement.prototype, 'clientHeight', realHeight) + }) + + it('FE-SHARED-MAP-001: frames the places for the card it sits in, not for the browser window', () => { + cardSize = { width: 750, height: 338 } + renderMap() + + const forCard = computeMapViewport(places, { tileSize: TILE_SIZE_RASTER, padding: PADDING, maxZoom: 14, width: 750, height: 338 })! + const forWindow = computeMapViewport(places, { tileSize: TILE_SIZE_RASTER, padding: PADDING, maxZoom: 14 })! + expect(mounted).toHaveLength(1) + expect(mounted[0].zoom).toBe(Math.floor(forCard.zoom)) + expect(mounted[0].center).toEqual(forCard.center) + // The window-sized frame is the over-zoomed one the issue saw. + expect(mounted[0].zoom).toBeLessThan(Math.floor(forWindow.zoom)) + // Opened framed, so there is nothing for the first fit to correct. + expect(fitBounds).not.toHaveBeenCalled() + }) + + it('FE-SHARED-MAP-002: opens on a whole zoom level, as fitBounds does', () => { + cardSize = { width: 750, height: 338 } + renderMap() + expect(Number.isInteger(mounted[0].zoom)).toBe(true) + }) + + it('FE-SHARED-MAP-003: a card without layout lets the first fit do the framing', () => { + cardSize = { width: 0, height: 0 } + renderMap() + expect(fitBounds).toHaveBeenCalledTimes(1) + expect(fitBounds.mock.calls[0][1]).toEqual({ paddingTopLeft: [48, 64], paddingBottomRight: [48, 48], maxZoom: 14 }) + }) + + it('FE-SHARED-MAP-004: a new set of places (a picked day) still refits with the same frame', () => { + cardSize = { width: 750, height: 338 } + const { rerender } = renderMap() + expect(fitBounds).not.toHaveBeenCalled() + rerender( + {}} />, + ) + expect(fitBounds).toHaveBeenCalledTimes(1) + expect(fitBounds.mock.calls[0][1]).toEqual({ paddingTopLeft: [48, 64], paddingBottomRight: [48, 48], maxZoom: 14 }) + }) +}) diff --git a/client/src/pages/sharedTrip/SharedMap.tsx b/client/src/pages/sharedTrip/SharedMap.tsx index 3ed817267..1f34e5776 100644 --- a/client/src/pages/sharedTrip/SharedMap.tsx +++ b/client/src/pages/sharedTrip/SharedMap.tsx @@ -1,5 +1,5 @@ import L from 'leaflet' -import { createElement, useEffect, useRef, type ReactNode } from 'react' +import { createElement, useEffect, useLayoutEffect, useRef, useState, type ReactNode } from 'react' import { ChevronLeft, ChevronRight } from 'lucide-react' import { MapContainer, Marker, Polyline, TileLayer, Tooltip, useMap } from 'react-leaflet' import MarkerClusterGroup from 'react-leaflet-cluster' @@ -55,6 +55,12 @@ function createMarkerIcon(place: MapPlace, orderNumbers?: number[] | null) { }) } +// One frame for both the opening camera and every later fit, so the map opens on +// exactly the view that picking a day and going back to the whole trip produces. +// The top edge leaves room for the day picker floating over the map. +const FRAME_PADDING = { top: 64, right: 48, bottom: 48, left: 48 } +const FRAME_MAX_ZOOM = 14 + function FitBoundsToPlaces({ places, framedOnMount }: { places: MapPlace[]; framedOnMount: boolean }) { const map = useMap() const fitRan = useRef(false) @@ -72,7 +78,11 @@ function FitBoundsToPlaces({ places, framedOnMount }: { places: MapPlace[]; fram } fitRan.current = true const bounds = L.latLngBounds(places.map(p => [p.lat, p.lng])) - map.fitBounds(bounds, { padding: [48, 48], maxZoom: 14 }) + map.fitBounds(bounds, { + paddingTopLeft: [FRAME_PADDING.left, FRAME_PADDING.top], + paddingBottomRight: [FRAME_PADDING.right, FRAME_PADDING.bottom], + maxZoom: FRAME_MAX_ZOOM, + }) }, [fitKey, map]) // eslint-disable-line react-hooks/exhaustive-deps return null } @@ -94,10 +104,31 @@ interface SharedMapProps { * map and the list can never disagree on which day is open. */ export function SharedMap({ places, line, orderByPlace, cartoApiKey, days, selectedDay, onSelectDay }: SharedMapProps) { + // The card's own size, read before the map mounts. Framing for the browser window + // instead zoomed a card a fraction of its size far too close, and the fit below + // then skipped the correction because it believed the map had opened framed (#2549). + const cardRef = useRef(null) + const [card, setCard] = useState<{ width: number; height: number } | null>(null) + useLayoutEffect(() => { + const el = cardRef.current + if (el) setCard({ width: el.clientWidth, height: el.clientHeight }) + }, []) + // A card with no layout yet (hidden, or a test DOM) has nothing to frame against, + // so the map opens near the places and the first fit does the real framing. + const measured = card !== null && card.width > 0 && card.height > 0 // Open framed on the trip's places instead of on Paris. MapContainer only reads // center/zoom at mount, so recomputing this per render is free. - const framed = computeMapViewport(places, { tileSize: TILE_SIZE_RASTER, padding: { top: 64, right: 48, bottom: 48, left: 48 } }) - const initialView = framed ?? { center: DEFAULT_MAP_CENTER, zoom: DEFAULT_MAP_ZOOM } + const framed = computeMapViewport(places, { + tileSize: TILE_SIZE_RASTER, + padding: FRAME_PADDING, + maxZoom: FRAME_MAX_ZOOM, + ...(measured ? { width: card.width, height: card.height } : {}), + }) + // Whole levels, as fitBounds settles on: Leaflet rounds a fractional opening zoom + // to the nearest level, and rounding up crops the outermost places. + const initialView = framed + ? { center: framed.center, zoom: Math.floor(framed.zoom) } + : { center: DEFAULT_MAP_CENTER, zoom: DEFAULT_MAP_ZOOM } // A visitor of a share link has no settings of their own, so the basemap is the // app default: OpenFreeMap, a vector style that needs no key at all. The owner's // CARTO key still travels in the payload and is still applied, because a raster @@ -107,8 +138,8 @@ export function SharedMap({ places, line, orderByPlace, cartoApiKey, days, selec return ( // `isolate` keeps Leaflet's pane z-indexes inside the card, under the sticky tab bar. -
- + {card && )} - + {selectedDay != null && line.length > 1 && ( [p.lat, p.lng])} @@ -142,7 +173,7 @@ export function SharedMap({ places, line, orderByPlace, cartoApiKey, days, selec ))} - + } {days.length > 0 && }