From f9f50bf106c1c511b60374543cdd32642fc072eb Mon Sep 17 00:00:00 2001 From: Eric Larson Date: Fri, 28 Aug 2026 13:47:47 -0500 Subject: [PATCH] Fix search map result interactions --- .../components/SearchResults.test.tsx | 96 +++++--------- .../components/search/MapResultView.test.tsx | 95 +++++++++++--- .../src/__tests__/pages/SearchPage.test.tsx | 4 +- frontend/src/components/SearchResults.tsx | 49 +------- .../search/MapResultView.client.tsx | 119 +++++++----------- frontend/src/pages/SearchPage.tsx | 15 ++- 6 files changed, 169 insertions(+), 209 deletions(-) diff --git a/frontend/src/__tests__/components/SearchResults.test.tsx b/frontend/src/__tests__/components/SearchResults.test.tsx index 40409cca..59bfcc65 100644 --- a/frontend/src/__tests__/components/SearchResults.test.tsx +++ b/frontend/src/__tests__/components/SearchResults.test.tsx @@ -321,71 +321,37 @@ describe('SearchResults Component', () => { expect(otherArticle).not.toHaveAttribute('aria-current'); }); - it.each([ - ['below the visible list', { top: 600, bottom: 700 }, 250, true], - ['above the visible list', { top: -200, bottom: -100 }, 0, true], - ['inside the visible list', { top: 100, bottom: 200 }, null, true], - [ - 'below the visible list when automatic scrolling is disabled', - { top: 600, bottom: 700 }, - null, - false, - ], - ])( - 'handles a highlighted compact result %s', - (_position, bounds, expectedScrollTop, autoScrollHighlightedResult) => { - const { rerender } = render( - - - - ); - - const highlightedArticle = screen - .getByText('Nondigitized paper map with library catalog link') - .closest('article') as HTMLElement; - const resultList = screen.getByTestId('map-results-scroll-container'); - const scrollTo = vi - .spyOn(window, 'scrollTo') - .mockImplementation(() => {}); - vi.spyOn(window, 'scrollY', 'get').mockReturnValue(50); - vi.spyOn(resultList, 'getBoundingClientRect').mockReturnValue({ - top: 0, - bottom: 500, - } as DOMRect); - vi.spyOn(highlightedArticle, 'getBoundingClientRect').mockReturnValue( - bounds as DOMRect - ); - - rerender( - - - - ); - - if (expectedScrollTop !== null) { - expect(scrollTo).toHaveBeenCalledWith({ - top: expectedScrollTop, - behavior: 'smooth', - }); - } else { - expect(scrollTo).not.toHaveBeenCalled(); - } - } - ); + it('does not scroll when a compact map result is highlighted', () => { + const scrollTo = vi + .spyOn(window, 'scrollTo') + .mockImplementation(() => {}); + const { rerender } = render( + + + + ); + + rerender( + + + + ); + + expect(scrollTo).not.toHaveBeenCalled(); + }); it('lets compact map results contribute their full height to the page', () => { render( diff --git a/frontend/src/__tests__/components/search/MapResultView.test.tsx b/frontend/src/__tests__/components/search/MapResultView.test.tsx index bd2b2369..06e857a5 100644 --- a/frontend/src/__tests__/components/search/MapResultView.test.tsx +++ b/frontend/src/__tests__/components/search/MapResultView.test.tsx @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { act, render, screen, waitFor } from '@testing-library/react'; import { MemoryRouter } from 'react-router'; import L from 'leaflet'; +import OverlappingMarkerSpiderfier from '@krozamdev/overlapping-marker-spiderfier'; import { MapProvider, useMap as useMapContext, @@ -195,6 +196,19 @@ function MapStateProbe() { ); } +function getOmsListener( + oms: MockOmsInstance, + eventName: string +): (marker: L.Marker) => void { + const listener = oms.addListener.mock.calls.find( + ([event]) => event === eventName + )?.[1]; + if (typeof listener !== 'function') { + throw new Error(`Missing ${eventName} listener`); + } + return listener as (marker: L.Marker) => void; +} + describe('MapResultView', () => { beforeEach(() => { vi.clearAllMocks(); @@ -268,9 +282,64 @@ describe('MapResultView', () => { const icon = mockOmsInstances[0].markers[0].options.icon as L.DivIcon; const iconHtml = String(icon.options.html); - expect(iconHtml).toContain('>11'); + expect(iconHtml).toContain('>11.'); expect(iconHtml).toContain('rotate(-45deg)'); expect(iconHtml).not.toContain('translateX(-50%) rotate(45deg)'); + expect(iconHtml).toContain('background: rgb(var(--color-primary));'); + expect(iconHtml).toContain('border: 2px solid #fff;'); + expect(iconHtml).not.toContain('#4f46e5'); + expect(mockOmsInstances[0].markers[0].options.title).toBe('Result 11'); + }); + + it('uses the spiderfier defaults', async () => { + render( + + + + ); + + await waitFor(() => { + expect(mockOmsInstances[0]?.markers).toHaveLength(2); + }); + + expect(OverlappingMarkerSpiderfier).toHaveBeenCalledWith(mockMap); + expect(mockOmsInstances[0].addListener).not.toHaveBeenCalledWith( + 'spiderfy', + expect.any(Function) + ); + }); + + it('uses the governed active blue when a pin is highlighted', async () => { + const { rerender } = render( + + + + ); + + await waitFor(() => { + expect(mockOmsInstances[0]?.markers).toHaveLength(2); + }); + + const marker = mockOmsInstances[0].markers[0]; + const markerElement = document.createElement('div'); + const pinShape = document.createElement('span'); + pinShape.setAttribute('data-result-pin-shape', ''); + markerElement.append(pinShape); + vi.spyOn(marker, 'getElement').mockReturnValue(markerElement); + + rerender( + + + + ); + + await waitFor(() => { + expect(pinShape.style.background).toBe('rgb(var(--color-active))'); + expect(pinShape.style.borderColor).toBe('rgb(var(--color-primary))'); + }); }); it('keeps the spiderfier stable while a marker is hovered and selected', async () => { @@ -292,7 +361,8 @@ describe('MapResultView', () => { expect(mapState).toHaveAttribute('data-hovered-resource-id', 'res-1'); expect(mapState).toHaveAttribute('data-hovered-resource-source', 'map'); - act(() => marker.fire('click')); + const handleOmsClick = getOmsListener(oms, 'click'); + act(() => handleOmsClick(marker)); expect(mapState).toHaveAttribute('data-selected-resource-id', 'res-1'); expect(mockOmsInstances).toHaveLength(1); expect(oms.clearMarkers).not.toHaveBeenCalled(); @@ -301,7 +371,7 @@ describe('MapResultView', () => { expect(mapState).toHaveAttribute('data-hovered-resource-id', 'res-1'); expect(mapState).toHaveAttribute('data-hovered-resource-source', 'map'); - act(() => marker.fire('click')); + act(() => handleOmsClick(marker)); expect(mapState).toHaveAttribute('data-selected-resource-id', ''); expect(mapState).toHaveAttribute('data-hovered-resource-id', ''); expect(mapState).toHaveAttribute('data-hovered-resource-source', ''); @@ -309,7 +379,7 @@ describe('MapResultView', () => { expect(oms.clearMarkers).not.toHaveBeenCalled(); }); - it('keeps collapsed overlapping markers stable until click', async () => { + it('delegates marker clicks to the spiderfier', async () => { render( @@ -322,23 +392,18 @@ describe('MapResultView', () => { }); const marker = mockOmsInstances[0].markers[0]; const mapState = screen.getByTestId('map-state'); - const fire = vi.spyOn(marker, 'fire'); - const setIcon = vi.spyOn(marker, 'setIcon'); act(() => marker.fire('mouseover')); - expect(fire).not.toHaveBeenCalledWith('click'); - expect(setIcon).not.toHaveBeenCalled(); - expect(mapState).toHaveAttribute('data-hovered-resource-id', ''); + expect(mapState).toHaveAttribute('data-hovered-resource-id', 'res-1'); + expect(mapState).toHaveAttribute('data-hovered-resource-source', 'map'); act(() => marker.fire('click')); expect(mapState).toHaveAttribute('data-selected-resource-id', ''); - marker._omsData = { - usualPosition: marker.getLatLng(), - leg: {} as L.Polyline, - }; - act(() => marker.fire('mouseover')); - expect(mapState).toHaveAttribute('data-hovered-resource-id', 'res-1'); + const handleOmsClick = getOmsListener(mockOmsInstances[0], 'click'); + act(() => handleOmsClick(marker)); + expect(mapState).toHaveAttribute('data-selected-resource-id', 'res-1'); + expect(mockMap.openPopup).toHaveBeenCalledTimes(1); }); it('accepts highlightedResourceId and highlightedGeometry', async () => { diff --git a/frontend/src/__tests__/pages/SearchPage.test.tsx b/frontend/src/__tests__/pages/SearchPage.test.tsx index b79f9d60..4eceea56 100644 --- a/frontend/src/__tests__/pages/SearchPage.test.tsx +++ b/frontend/src/__tests__/pages/SearchPage.test.tsx @@ -405,8 +405,10 @@ describe('SearchPage Logic', () => { expect(map).toContainElement(note); expect(note).toHaveTextContent( - 'This map only renders the current page of search results. Use pagination to review other matches.' + 'This map shows the current page of search results' ); + expect(note.querySelector('.lucide-info')).toBeInTheDocument(); + expect(note).toHaveClass('lg:w-auto', 'lg:whitespace-nowrap'); expect(resultsSummary).toContainElement(pagination); expect(pagination).toHaveClass('whitespace-nowrap'); expect(resultsColumn).not.toContainElement(pagination); diff --git a/frontend/src/components/SearchResults.tsx b/frontend/src/components/SearchResults.tsx index 8dff57da..7435769a 100644 --- a/frontend/src/components/SearchResults.tsx +++ b/frontend/src/components/SearchResults.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react'; +import { useState } from 'react'; import { Link, useLocation } from 'react-router'; import type { GeoDocument } from '../types/api'; import { BookOpen } from 'lucide-react'; @@ -27,7 +27,6 @@ interface SearchResultsProps { searchId?: string; searchView?: 'list' | 'gallery' | 'map'; highlightedResourceId?: string | null; - autoScrollHighlightedResult?: boolean; } export function SearchResults({ @@ -40,7 +39,6 @@ export function SearchResults({ searchId, searchView = 'list', highlightedResourceId = null, - autoScrollHighlightedResult = false, }: SearchResultsProps) { const { showDetails } = useDebug(); const location = useLocation(); @@ -52,8 +50,6 @@ export function SearchResults({ } = useMap(); const { isBookmarked } = useBookmarks(); const [imageErrors, setImageErrors] = useState>(new Set()); - const resultListRef = useRef(null); - const resultCardRefs = useRef>(new Map()); const isCompact = variant === 'compact'; const thumbnailWrapperClass = isCompact ? 'w-24' : 'w-24 md:w-48'; @@ -66,41 +62,6 @@ export function SearchResults({ ? 'text-sm line-clamp-2' : 'text-sm line-clamp-2 md:text-xl'; - useEffect(() => { - if (!isCompact || !highlightedResourceId || !autoScrollHighlightedResult) - return; - - const highlightedCard = resultCardRefs.current.get(highlightedResourceId); - const resultList = resultListRef.current; - if (!highlightedCard || !resultList) return; - - const cardBounds = highlightedCard.getBoundingClientRect(); - const listBounds = resultList.getBoundingClientRect(); - const viewportHeight = - window.innerHeight || document.documentElement.clientHeight; - const headerBottom = - document.querySelector('header')?.getBoundingClientRect().bottom ?? 0; - const visibleListTop = Math.max(listBounds.top, headerBottom, 0); - const visibleListBottom = Math.min(listBounds.bottom, viewportHeight); - if (visibleListBottom <= visibleListTop) return; - - let scrollDelta = 0; - if (cardBounds.top < visibleListTop) { - scrollDelta = cardBounds.top - visibleListTop; - } else if (cardBounds.bottom > visibleListBottom) { - scrollDelta = cardBounds.bottom - visibleListBottom; - } - - if (scrollDelta !== 0) { - const targetScrollTop = Math.max(0, window.scrollY + scrollDelta); - - window.scrollTo({ - top: targetScrollTop, - behavior: 'smooth', - }); - } - }, [autoScrollHighlightedResult, highlightedResourceId, isCompact]); - // Calculate absolute index in full result set (1-based) const getAbsoluteIndex = (relativeIndex: number) => { return (currentPage - 1) * perPage + relativeIndex + 1; @@ -149,7 +110,6 @@ export function SearchResults({ return (
{ - if (element) { - resultCardRefs.current.set(result.id, element); - } else { - resultCardRefs.current.delete(result.id); - } - }} className={`min-w-0 bg-white rounded-lg shadow-md hover:shadow-lg transition-all relative group ${ isHighlighted ? 'ring-2 ring-blue-500/80 bg-blue-50 shadow-md' diff --git a/frontend/src/components/search/MapResultView.client.tsx b/frontend/src/components/search/MapResultView.client.tsx index a120a026..ebe3c624 100644 --- a/frontend/src/components/search/MapResultView.client.tsx +++ b/frontend/src/components/search/MapResultView.client.tsx @@ -212,7 +212,8 @@ const MapInitialFitController: React.FC<{ /** Create a numbered map pin icon with an upright result number. */ function createNumberedPinIcon(resultNumber: number): L.DivIcon { const size = 26; - const textSize = 11; + const label = `${resultNumber}.`; + const textSize = label.length >= 4 ? 9 : label.length === 3 ? 10 : 11; const markerHeight = Math.round(size * 1.35); return L.divIcon({ html: `${resultNumber} + " data-result-pin-label>${label} `, className: 'numbered-pin-icon', iconSize: [size, markerHeight], @@ -263,8 +264,6 @@ function createNumberedPinIcon(resultNumber: number): L.DivIcon { interface MarkerEntry { marker: L.Marker; resourceId: string; - resultNumber: number; - isCollapsedOverlappingMarker: () => boolean; } interface PinData { @@ -274,27 +273,23 @@ interface PinData { hoverGeometry: string | null; } -const SPIDERFY_NEARBY_DISTANCE = 30; - function updateMarkerPresentation( entry: MarkerEntry, highlightedResourceId: string | null ) { const isHighlighted = entry.resourceId === highlightedResourceId; - const isCollapsedOverlap = entry.isCollapsedOverlappingMarker(); const element = entry.marker.getElement(); if (element) { const shape = element.querySelector('[data-result-pin-shape]'); if (shape) { - shape.style.background = isHighlighted ? '#f59e0b' : '#4f46e5'; - shape.style.borderColor = isHighlighted ? '#7c3aed' : '#312e81'; + shape.style.background = isHighlighted + ? 'rgb(var(--color-active))' + : 'rgb(var(--color-primary))'; + shape.style.borderColor = isHighlighted + ? 'rgb(var(--color-primary))' + : '#fff'; } - - element.title = isCollapsedOverlap - ? `Result ${entry.resultNumber} overlaps nearby results. Click to separate.` - : `Result ${entry.resultNumber}`; - element.style.cursor = isCollapsedOverlap ? 'zoom-in' : 'pointer'; } entry.marker.setZIndexOffset(isHighlighted ? 10000 : 0); @@ -317,6 +312,7 @@ const SpiderfiedMarkers: React.FC<{ typeof OverlappingMarkerSpiderfier > | null>(null); const entriesRef = useRef([]); + const markerDataRef = useRef>(new Map()); const highlightedResourceIdRef = useRef(highlightedResourceId); highlightedResourceIdRef.current = highlightedResourceId; const selectedResourceIdRef = useRef(selectedResourceId); @@ -325,74 +321,75 @@ const SpiderfiedMarkers: React.FC<{ useEffect(() => { if (!map) return; - const oms = new OverlappingMarkerSpiderfier(map, { - nearbyDistance: SPIDERFY_NEARBY_DISTANCE, - circleSpiralSwitchover: 9, - }); + const oms = new OverlappingMarkerSpiderfier(map); omsRef.current = oms; const popup = L.popup(); const handleMarkerClick = ( marker: L.Marker & { _popupContent?: HTMLElement } ) => { + const pin = markerDataRef.current.get(marker); + if (!pin) return; + if (marker._popupContent) { popup.setContent(marker._popupContent); popup.setLatLng(marker.getLatLng()); map.openPopup(popup); } + + const nextSelected = + selectedResourceIdRef.current === pin.resource.id + ? null + : pin.resource.id; + selectedResourceIdRef.current = nextSelected; + setSelectedResourceId(nextSelected); + setHoveredResourceSource(nextSelected ? 'map' : null); + setHoveredResourceId(nextSelected); + setHoveredGeometry(nextSelected ? pin.hoverGeometry : null); }; oms.addListener('click', handleMarkerClick); + const refreshMarkerPresentations = () => { entriesRef.current.forEach((entry) => updateMarkerPresentation(entry, highlightedResourceIdRef.current) ); }; - oms.addListener('spiderfy', refreshMarkerPresentations); oms.addListener('unspiderfy', refreshMarkerPresentations); return () => { oms.removeListener('click', handleMarkerClick); - oms.removeListener('spiderfy', refreshMarkerPresentations); oms.removeListener('unspiderfy', refreshMarkerPresentations); oms.clearMarkers(); - oms.unspiderfy(); + markerDataRef.current.clear(); if (omsRef.current === oms) { omsRef.current = null; } }; - }, [map]); + }, [ + map, + setHoveredResourceId, + setHoveredResourceSource, + setHoveredGeometry, + setSelectedResourceId, + ]); useEffect(() => { const oms = omsRef.current; if (!map || !oms || pins.length === 0) { entriesRef.current = []; + markerDataRef.current.clear(); return; } + const markerData = new Map(); + markerDataRef.current = markerData; const entries: MarkerEntry[] = []; pins.forEach((p) => { - const hasNearbyMarker = () => { - try { - const markerPoint = map.latLngToLayerPoint(L.latLng(p.position)); - const nearbyDistanceSquared = SPIDERFY_NEARBY_DISTANCE ** 2; - - return pins.some((otherPin) => { - if (otherPin === p) return false; - const otherPoint = map.latLngToLayerPoint( - L.latLng(otherPin.position) - ); - const deltaX = markerPoint.x - otherPoint.x; - const deltaY = markerPoint.y - otherPoint.y; - return deltaX ** 2 + deltaY ** 2 < nearbyDistanceSquared; - }); - } catch { - return false; - } - }; - const marker = L.marker(p.position, { icon: createNumberedPinIcon(p.resultNumber), + title: `Result ${p.resultNumber}`, }); + markerData.set(marker, p); const container = document.createElement('div'); container.className = 'text-xs min-w-[200px]'; const resultLabel = document.createElement('span'); @@ -417,12 +414,7 @@ const SpiderfiedMarkers: React.FC<{ (marker as L.Marker & { _popupContent?: HTMLElement })._popupContent = container; - const isCollapsedOverlappingMarker = () => { - return !marker._omsData && hasNearbyMarker(); - }; - marker.on('mouseover', () => { - if (isCollapsedOverlappingMarker()) return; setHoveredResourceSource('map'); setHoveredResourceId(p.resource.id); setHoveredGeometry(p.hoverGeometry); @@ -435,44 +427,27 @@ const SpiderfiedMarkers: React.FC<{ setHoveredGeometry(null); } }); - - marker.on('click', () => { - if (isCollapsedOverlappingMarker()) return; - const nextSelected = - selectedResourceIdRef.current === p.resource.id - ? null - : p.resource.id; - selectedResourceIdRef.current = nextSelected; - setSelectedResourceId(nextSelected); - setHoveredResourceSource(nextSelected ? 'map' : null); - setHoveredResourceId(nextSelected); - setHoveredGeometry(nextSelected ? p.hoverGeometry : null); - }); marker.addTo(map); oms.addMarker(marker); entries.push({ marker, resourceId: p.resource.id, - resultNumber: p.resultNumber, - isCollapsedOverlappingMarker, }); }); entriesRef.current = entries; - const refreshMarkerPresentations = () => { - entries.forEach((entry) => - updateMarkerPresentation(entry, highlightedResourceIdRef.current) - ); - }; - map.on('zoomend moveend', refreshMarkerPresentations); - refreshMarkerPresentations(); + entries.forEach((entry) => + updateMarkerPresentation(entry, highlightedResourceIdRef.current) + ); return () => { - map.off('zoomend moveend', refreshMarkerPresentations); oms.clearMarkers(); entries.forEach((entry) => map.removeLayer(entry.marker)); if (entriesRef.current === entries) { entriesRef.current = []; } + if (markerDataRef.current === markerData) { + markerDataRef.current.clear(); + } }; }, [ map, diff --git a/frontend/src/pages/SearchPage.tsx b/frontend/src/pages/SearchPage.tsx index 7b364c5f..13d2b4cd 100644 --- a/frontend/src/pages/SearchPage.tsx +++ b/frontend/src/pages/SearchPage.tsx @@ -8,7 +8,7 @@ import { Header } from '../components/layout/Header'; import { Footer } from '../components/layout/Footer'; import type { AdvancedClause, FacetFilter } from '../types/search'; import { FacetList } from '../components/FacetList'; -import { SlidersHorizontal, X } from 'lucide-react'; +import { Info, SlidersHorizontal, X } from 'lucide-react'; // import { MapView } from '../components/search/MapView'; import { MapProvider, useMap } from '../context/MapContext'; import { SortControl } from '../components/search/SortControl'; @@ -66,7 +66,6 @@ function SearchContent({ }: SearchPageProps) { const { hoveredResourceId, - hoveredResourceSource, hoveredGeometry, setHoveredResourceId, setHoveredResourceSource, @@ -877,9 +876,6 @@ function SearchContent({ searchId={searchId} searchView={currentView} highlightedResourceId={activeMapResourceId} - autoScrollHighlightedResult={ - hoveredResourceSource === 'map' - } />
@@ -891,11 +887,14 @@ function SearchContent({ {!activeIsLoading && ( )}