diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 34e41a5..cdae421 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -500,12 +500,8 @@ test('results map fullscreen falls back to an overlay on iOS', async ({ page }) delete Element.prototype.requestFullscreen; }); + // A postcode search opens on the map, phones included; open it fullscreen. await searchByName(page, 'B1 1BB'); - await expect(schoolLinks(page).first()).toBeVisible({ timeout: 15_000 }); - - // Switch to the map view with the floating button (the toolbar's switch is - // hidden at phone width), then open the map fullscreen. - await page.getByRole('button', { name: 'Show map' }).click(); const openFs = page.getByRole('button', { name: 'View map fullscreen' }); await expect(openFs).toBeVisible({ timeout: 15_000 }); await openFs.click(); @@ -581,26 +577,29 @@ test('a desktop postcode search opens on the map with the list beside it', async }); for (const width of [360, 390, 430]) { - test(`the floating Map button is in reach at ${width}px`, async ({ page }) => { + test(`a phone opens on the map, with the list a tap away, at ${width}px`, async ({ page }) => { await page.setViewportSize({ width, height: 800 }); await searchByName(page, 'B1 1BB'); - await expect(schoolLinks(page).first()).toBeVisible({ timeout: 15_000 }); - // Visible without scrolling, and clear of the bottom tab bar. - const fab = page.getByRole('button', { name: 'Show map' }); - await expect(fab).toBeInViewport(); - const fabBox = (await fab.boundingBox())!; + // On the map, with the floating button offering the list, clear of the + // bottom tab bar. + const toList = page.getByRole('button', { name: 'Show list' }); + await expect(toList).toBeInViewport({ timeout: 15_000 }); + await expect(page.locator('.sc-pin').first()).toBeAttached({ timeout: 15_000 }); const barTop = await page.locator('nav[class*="bottomBar"]') .evaluate((el) => el.getBoundingClientRect().top); + const fabBox = (await toList.boundingBox())!; expect(fabBox.y + fabBox.height).toBeLessThanOrEqual(barTop); - // The search folds to a summary, and the pinned toolbar survives a scroll. - const summary = page.getByRole('button', { name: /^Edit search: B1 1BB/ }); - await expect(summary).toBeVisible(); - await page.evaluate(() => window.scrollTo(0, 1200)); - await expect.poll(() => page.evaluate(() => window.scrollY)).toBeGreaterThan(600); - await expect(summary).toBeInViewport(); - await expect(fab).toBeInViewport(); + // A pin opens the bottom sheet, stacked under the button, above the bar. + // dispatchEvent, not click: a pin may sit under the button or the toolbar, + // and Leaflet listens on the pin itself. + await page.locator('.sc-pin').first().dispatchEvent('click'); + const sheet = page.locator('[class*="bottomSheet"]'); + await expect(sheet).toBeVisible(); + const sheetBox = (await sheet.boundingBox())!; + expect(sheetBox.y + sheetBox.height).toBeLessThanOrEqual(barTop); + expect(sheetBox.y).toBeGreaterThanOrEqual(fabBox.y + fabBox.height); // MOBILE.md: no horizontal overflow, and 44px targets in the new chrome. expect(await page.evaluate(() => document.documentElement.scrollWidth - window.innerWidth)) @@ -616,8 +615,17 @@ for (const width of [360, 390, 430]) { }); expect(small).toEqual([]); - await fab.click(); - await expect(page.getByRole('button', { name: 'Show list' })).toBeVisible(); + // The list: the search folds to a summary, and the pinned toolbar and + // the button survive a scroll. + await toList.click(); + const toMap = page.getByRole('button', { name: 'Show map' }); + await expect(toMap).toBeInViewport(); + await expect(schoolLinks(page).first()).toBeVisible(); + const summary = page.getByRole('button', { name: /^Edit search: B1 1BB/ }); + await page.evaluate(() => window.scrollTo(0, 1200)); + await expect.poll(() => page.evaluate(() => window.scrollY)).toBeGreaterThan(600); + await expect(summary).toBeInViewport(); + await expect(toMap).toBeInViewport(); }); } diff --git a/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx b/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx index 4b35b3f..9fb0bfe 100644 --- a/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx +++ b/nextjs-app/__tests__/components/HomeView.staleFetch.test.tsx @@ -39,15 +39,17 @@ beforeEach(() => { }); test('load-more results from an old search are discarded, even after returning to it', async () => { + // Name searches: a postcode search opens on the map, which has no Load more. + params = new URLSearchParams('search=abbey'); const pending = deferred(); jest.mocked(fetchSchools).mockReturnValueOnce(pending.promise); const view = render(); fireEvent.click(screen.getByRole('button', { name: 'Load more schools' })); const signal = jest.mocked(fetchSchools).mock.calls[0][1]?.signal; - params = new URLSearchParams('postcode=SW2+1AA'); + params = new URLSearchParams('search=brecknock'); view.rerender(); expect(signal?.aborted).toBe(true); - params = new URLSearchParams('postcode=SW1A+1AA'); + params = new URLSearchParams('search=abbey'); view.rerender(); await act(async () => pending.resolve(response('Stale append'))); expect(screen.queryByText('Stale append')).not.toBeInTheDocument(); diff --git a/nextjs-app/__tests__/components/LeafletMapInner.test.tsx b/nextjs-app/__tests__/components/LeafletMapInner.test.tsx index 8f8eb16..5a47ae0 100644 --- a/nextjs-app/__tests__/components/LeafletMapInner.test.tsx +++ b/nextjs-app/__tests__/components/LeafletMapInner.test.tsx @@ -95,3 +95,13 @@ it('opens no card on a narrow screen, where the page shows a bottom sheet', () = expect(container.querySelectorAll('.sc-pin--selected')).toHaveLength(1); expect(container.querySelector('.sc-popup')).toBeNull(); }); + +it('puts the card back when the pins are rebuilt for a reason other than the schools', () => { + const onDeselect = jest.fn(); + const { container, rerender } = renderMap({ onDeselect, selectedUrn: 1 }); + act(() => rerender({ onDeselect, selectedUrn: 1, radiusMiles: 3, referencePoint: [51.43, -0.21] })); + expect(onDeselect).not.toHaveBeenCalled(); + expect(container.querySelector('.sc-radius-label')).toHaveTextContent('3 miles'); + expect(container.querySelector('.sc-popup')).toHaveTextContent('Southmead Primary School'); + expect(container.querySelectorAll('.sc-pin--selected')).toHaveLength(1); +}); diff --git a/nextjs-app/__tests__/components/ResultsMapView.test.tsx b/nextjs-app/__tests__/components/ResultsMapView.test.tsx index c618f3c..6045375 100644 --- a/nextjs-app/__tests__/components/ResultsMapView.test.tsx +++ b/nextjs-app/__tests__/components/ResultsMapView.test.tsx @@ -5,8 +5,8 @@ import { primaryFixture } from '../support/schoolFixtures'; import type { School, SchoolsResponse } from '@/lib/types'; /* - * The map view: the list beside the map (mockup B), opening on the map for - * desktop. The map itself is Leaflet and mocked here; what is pinned is what + * The map view: the list beside the map (mockup B), where every postcode + * search opens. The map itself is Leaflet and mocked here; what is pinned is what * HomeView hands it and the list it draws beside it. */ @@ -63,26 +63,33 @@ beforeEach(() => { params = new URLSearchParams('postcode=SW196AR&radius=1'); jest.mocked(fetchSchools).mockReset().mockResolvedValue(results()); jest.mocked(fetchNationalAverages).mockResolvedValue({ primary: { rwm_expected_pct: 62 } } as never); - window.innerWidth = 1440; + setWide(true); }); +/** Desktop unless a test says otherwise: the list pane is shown from 769px. */ +function setWide(wide: boolean) { + window.matchMedia = ((q: string) => ({ + matches: wide, media: q, addEventListener() {}, removeEventListener() {}, + })) as unknown as typeof window.matchMedia; +} + async function renderMap() { - const view = render(); + const view = render(); await act(async () => {}); return view; } -it('opens on the map when the server says desktop', async () => { +it('opens a postcode search on the map', async () => { await renderMap(); expect(screen.getByTestId('map')).toHaveAttribute('data-radius', '1'); expect(screen.getByRole('button', { name: 'Map' })).toHaveAttribute('aria-pressed', 'true'); }); -it('falls back to the list in a window too narrow for the split', async () => { - window.innerWidth = 800; - await renderMap(); +it('lists a name search, which has no map', async () => { + params = new URLSearchParams('search=southmead'); + render(); + await act(async () => {}); expect(screen.queryByTestId('map')).not.toBeInTheDocument(); - expect(screen.getByRole('button', { name: 'List' })).toHaveAttribute('aria-pressed', 'true'); }); it('puts the count and the sort in the list beside the map, once', async () => { @@ -119,22 +126,29 @@ it('shows the England comparison for mainstream schools only, and never a placeh expect(card(3)).not.toHaveTextContent(/%|pts/); }); -it('follows a new default after a client-side search, until the reader picks', async () => { +it('opens the map after a hero search, and keeps the reader\'s choice after that', async () => { + // Landing page, then a hero search: the same instance gets new props. params = new URLSearchParams(''); const empty = { schools: [], total: 0, page: 1, page_size: 25, total_pages: 0 } as SchoolsResponse; - const view = render(); - - // Hero search: same component, new props. + const view = render(); params = new URLSearchParams('postcode=SW196AR&radius=1'); - view.rerender(); + view.rerender(); await act(async () => {}); expect(screen.getByTestId('map')).toBeInTheDocument(); - // Chosen: a later search keeps the reader's view. + // Chosen: a later search keeps the list. fireEvent.click(screen.getByRole('button', { name: 'List' })); params = new URLSearchParams('postcode=SW170AA&radius=1'); - view.rerender(); - view.rerender(); + view.rerender(); await act(async () => {}); expect(screen.queryByTestId('map')).not.toBeInTheDocument(); }); + +it('builds no list cards on a phone, where the pane is hidden', async () => { + setWide(false); + const { container } = await renderMap(); + expect(screen.getByTestId('map')).toBeInTheDocument(); + expect(container.querySelectorAll('[data-urn]')).toHaveLength(0); + // The count stays: it is the pane's heading, shown above the map. + expect(screen.getByRole('heading', { name: /3 schools within/ })).toBeInTheDocument(); +}); diff --git a/nextjs-app/__tests__/components/ResultsToolbar.test.tsx b/nextjs-app/__tests__/components/ResultsToolbar.test.tsx index 4b50b2a..563a252 100644 --- a/nextjs-app/__tests__/components/ResultsToolbar.test.tsx +++ b/nextjs-app/__tests__/components/ResultsToolbar.test.tsx @@ -54,25 +54,28 @@ describe('the List/Map switch', () => { render(); const view = screen.getByRole('group', { name: 'Results view' }); expect(view.closest('div[class*="resultsToolbar"]')).not.toBeNull(); - expect(screen.getByRole('button', { name: 'List' })).toHaveAttribute('aria-pressed', 'true'); - expect(screen.getByRole('button', { name: 'Map' })).toHaveAttribute('aria-pressed', 'false'); + // A postcode search opens on the map. + expect(screen.getByRole('button', { name: 'Map' })).toHaveAttribute('aria-pressed', 'true'); + expect(screen.getByRole('button', { name: 'List' })).toHaveAttribute('aria-pressed', 'false'); }); it('has a floating twin that flips between map and list', async () => { render(); - await act(async () => fireEvent.click(screen.getByRole('button', { name: 'Show map' }))); - expect(screen.getByTestId('map')).toBeInTheDocument(); - expect(screen.getByRole('button', { name: 'Map' })).toHaveAttribute('aria-pressed', 'true'); - expect(track).toHaveBeenCalledWith('results_view_changed', { view: 'map', via: 'floating' }); - + await act(async () => {}); fireEvent.click(screen.getByRole('button', { name: 'Show list' })); expect(screen.queryByTestId('map')).not.toBeInTheDocument(); - expect(screen.getByRole('button', { name: 'Show map' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'List' })).toHaveAttribute('aria-pressed', 'true'); + expect(track).toHaveBeenCalledWith('results_view_changed', { view: 'list', via: 'floating' }); + + await act(async () => fireEvent.click(screen.getByRole('button', { name: 'Show map' }))); + expect(screen.getByTestId('map')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Show list' })).toBeInTheDocument(); }); - it('does not track a click on the view already showing', () => { + it('does not track a click on the view already showing', async () => { render(); - fireEvent.click(screen.getByRole('button', { name: 'List' })); + await act(async () => {}); + fireEvent.click(screen.getByRole('button', { name: 'Map' })); expect(track).not.toHaveBeenCalledWith('results_view_changed', expect.anything()); }); diff --git a/nextjs-app/__tests__/lib/device.test.ts b/nextjs-app/__tests__/lib/device.test.ts deleted file mode 100644 index d1bfa33..0000000 --- a/nextjs-app/__tests__/lib/device.test.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { isHandheldUserAgent } from '@/lib/device'; - -describe('isHandheldUserAgent', () => { - it.each([ - ['iPhone Safari', 'Mozilla/5.0 (iPhone; CPU iPhone OS 17_5 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.5 Mobile/15E148 Safari/604.1'], - ['Android Chrome', 'Mozilla/5.0 (Linux; Android 14; Pixel 8) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0 Mobile Safari/537.36'], - ['Android tablet', 'Mozilla/5.0 (Linux; Android 13; SM-X710) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0 Safari/537.36'], - ])('reads %s as handheld', (_, ua) => { - expect(isHandheldUserAgent(ua)).toBe(true); - }); - - it.each([ - ['desktop Chrome', 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0 Safari/537.36'], - ['Windows Edge', 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0 Safari/537.36 Edg/126.0'], - ['missing', null], - ])('reads %s as not handheld', (_, ua) => { - expect(isHandheldUserAgent(ua)).toBe(false); - }); -}); diff --git a/nextjs-app/app/(frontend)/page.tsx b/nextjs-app/app/(frontend)/page.tsx index a55fcf8..378ae99 100644 --- a/nextjs-app/app/(frontend)/page.tsx +++ b/nextjs-app/app/(frontend)/page.tsx @@ -5,8 +5,6 @@ import { absoluteUrl } from '@/lib/site'; import type { Metadata } from 'next'; -import { headers } from 'next/headers'; -import { isHandheldUserAgent } from '@/lib/device'; import { fetchSchools, fetchFilters, fetchDataInfo } from '@/lib/api'; import { formatAcademicYear } from '@/lib/utils'; import { HomeView } from '@/components/HomeView'; @@ -116,20 +114,9 @@ export default async function HomePage({ searchParams }: HomePageProps) { // endpoint returns, and reading it silently yielded null on every request. const total = dataInfo?.unique_schools ?? null; const years = dataInfo?.years_available ?? []; - /* - * A postcode search opens on the map for desktop browsers and on the list - * for phones and tablets. The user agent is the only signal available - * before the first paint; HomeView falls back to the list if the window - * turns out too narrow, so a wrong guess costs one swap, not a broken page. - */ - const initialView = params.postcode - && !isHandheldUserAgent((await headers()).get('user-agent')) - ? 'map' : 'list'; - return ( (initialView); - // Once someone picks a view it is theirs; the default never overrides it. - const [viewChosen, setViewChosen] = useState(false); /* - * Follow the server's default until then. Most searches start in the hero - * and reach the results by client-side navigation, which hands this same - * component a new initialView rather than mounting a fresh one, so the - * initial state alone would leave a desktop reader on the list. Adjusted - * during render, not in an effect, so the list never paints first. + * The reader's choice, once they make one; until then the default for the + * kind of search. Derived rather than seeded into state, because a hero + * search reaches the results by client-side navigation: the same instance + * gets new props, and a state seeded on the landing page would stay "list". */ - const [defaultFor, setDefaultFor] = useState(initialView); - if (defaultFor !== initialView) { - setDefaultFor(initialView); - if (!viewChosen) { - setResultsView(initialView === 'map' && window.innerWidth < MAP_DEFAULT_MIN_WIDTH ? 'list' : initialView); - } - } - // The user agent said desktop, but the window is too narrow for the split - // layout (a narrow desktop window, or a test at phone width): fall back to - // the list before anyone has chosen. - useEffect(() => { - if (initialView === 'map' && window.innerWidth < MAP_DEFAULT_MIN_WIDTH) { - setResultsView('list'); - } - // Mount only; later defaults are handled during render above. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + const [chosenView, setChosenView] = useState<'list' | 'map' | null>(null); const [selectedMapSchool, setSelectedMapSchool] = useState(null); const sortOrder = searchParams.get('sort') || 'default'; const [allSchools, setAllSchools] = useState(initialSchools.schools); @@ -320,6 +298,7 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed const isSecondaryView = currentPhase.toLowerCase().includes('secondary') || (!currentPhase && secondaryCount > primaryCount); const isMixedView = primaryCount > 0 && secondaryCount > 0 && !currentPhase; + const resultsView: 'list' | 'map' = chosenView ?? (isLocationSearch ? DEFAULT_LOCATION_VIEW : 'list'); // The map view fills the screen below the pinned toolbar, whose height // depends on how its controls wrap. Measure it rather than guess. @@ -530,6 +509,22 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed const hasViewSwitch = isLocationSearch && initialSchools.schools.length > 0; const compareUrns = useMemo(() => selectedSchools.map(s => s.urn), [selectedSchools]); + + /* + * Below 769px the map view hides its list pane (HomeView.module.css), and + * with the map now the default on phones, every postcode search there + * would otherwise build up to 500 hidden cards. Decided after mount, so + * the server's HTML and the first client render still agree. + */ + const [listPaneShown, setListPaneShown] = useState(true); + useEffect(() => { + if (typeof window.matchMedia !== 'function') return; + const query = window.matchMedia('(min-width: 769px)'); + const update = () => setListPaneShown(query.matches); + update(); + query.addEventListener('change', update); + return () => query.removeEventListener('change', update); + }, []); const clearMapSelection = useCallback(() => setSelectedMapSchool(null), []); // A pin chosen on the map brings its card into view in the list beside it. @@ -547,8 +542,7 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed */ const changeView = (view: 'list' | 'map', via: 'toolbar' | 'floating') => { if (view === resultsView) return; - setResultsView(view); - setViewChosen(true); + setChosenView(view); track('results_view_changed', { view, via }); requestAnimationFrame(() => { const results = resultsRef.current; @@ -803,7 +797,7 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed
{resultsHeader}
- {mapListSchools.map((school) => ( + {listPaneShown && mapListSchools.map((school) => ( (null); const popupRef = useRef(null); const selectedRef = useRef(null); + // Bumped whenever the pins are rebuilt, for any reason, so the selection + // effect puts the card back on the new pin rather than only when the + // selection or the school list changes. + const [pinsVersion, setPinsVersion] = useState(0); // The popup is plain HTML outside React, so its handlers read the latest // props through a ref rather than closing over the render they were made in. @@ -216,6 +220,7 @@ export default function LeafletMapInner({ } else { map.setView(center, zoom); } + setPinsVersion(v => v + 1); }, [schools, center, zoom, referencePoint, radiusMiles]); // Selection: restyle the pin, and on wide screens open its card. @@ -267,7 +272,7 @@ export default function LeafletMapInner({ if ((e.target as HTMLElement).closest('[data-compare]')) latest.current.onAddToCompare?.(school); }); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [selectedUrn, schools]); + }, [selectedUrn, pinsVersion]); // The open card follows the compare basket and the averages as they arrive. useEffect(() => { diff --git a/nextjs-app/components/SchoolMap.tsx b/nextjs-app/components/SchoolMap.tsx index 7f4a94a..a839daa 100644 --- a/nextjs-app/components/SchoolMap.tsx +++ b/nextjs-app/components/SchoolMap.tsx @@ -6,7 +6,7 @@ 'use client'; import dynamic from 'next/dynamic'; -import { useRef, useState, useEffect, useCallback } from 'react'; +import { useRef, useState, useEffect, useCallback, useMemo } from 'react'; import type { School } from '@/lib/types'; import styles from './SchoolMap.module.css'; @@ -85,8 +85,9 @@ export function SchoolMap({ schools, center, zoom = 13, referencePoint, onMarker } }, [fallbackFullscreen]); - // Calculate center if not provided - const mapCenter: [number, number] = center || (() => { + // Calculate center if not provided. Memoised: a fresh array on every render + // would make the map refit and rebuild every pin each time. + const mapCenter = useMemo<[number, number]>(() => center || (() => { if (schools.length === 0) return [51.5074, -0.1278]; if (schools.length === 1 && schools[0].latitude && schools[0].longitude) { return [schools[0].latitude, schools[0].longitude]; @@ -95,8 +96,8 @@ export function SchoolMap({ schools, center, zoom = 13, referencePoint, onMarker if (validSchools.length === 0) return [51.5074, -0.1278]; const avgLat = validSchools.reduce((sum, s) => sum + (s.latitude || 0), 0) / validSchools.length; const avgLng = validSchools.reduce((sum, s) => sum + (s.longitude || 0), 0) / validSchools.length; - return [avgLat, avgLng]; - })(); + return [avgLat, avgLng] as [number, number]; + })(), [center, schools]); return (
diff --git a/nextjs-app/lib/device.ts b/nextjs-app/lib/device.ts deleted file mode 100644 index 509f1b7..0000000 --- a/nextjs-app/lib/device.ts +++ /dev/null @@ -1,16 +0,0 @@ -/** - * Whether a User-Agent string belongs to a phone or tablet. - * - * Used for one thing: choosing the results view before the first paint, when - * the viewport is not yet known. A wrong answer is corrected client-side (see - * HomeView's MAP_DEFAULT_MIN_WIDTH), so this errs towards "handheld" and is - * kept deliberately small rather than a full UA parser. - * - * iPadOS reports a desktop Mac UA by default and so reads as desktop here, - * which is the right call: a landscape iPad has room for the map layout. - */ -const HANDHELD = /Mobi|Android|iPhone|iPad|iPod|Tablet|Silk|Kindle|PlayBook|BlackBerry|Opera Mini|IEMobile/i; - -export function isHandheldUserAgent(ua: string | null | undefined): boolean { - return !!ua && HANDHELD.test(ua); -}