From ca4ddd2b12a205932f21abf9c1b5b6fc5ef6986e Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 1 Oct 2026 09:09:47 +0100 Subject: [PATCH] fix(search): address review on the shared result rows - The rows' narrow layout now switches at a 608px list, not 600px. Below 769px the page pads 1rem each side, so a 640px screen gives a 608px list: exactly the old max-width: 640px media query, where 600px left 633-640px screens on the wide layout. - rowContainerGuard.test.ts fails if anything other than HomeView renders SchoolRow or SecondarySchoolRow, or if one of HomeView's row lists loses its `results` container. Outside one the rows silently keep their wide layout on phones. (Checked: HomeView is the only importer today.) - Picking a pin from the list beside the map now works from the keyboard: each row carries a "Show on the map" button, visually hidden until focused, with aria-pressed for the selected school. The row itself cannot be the button, since it holds links and buttons of its own. Co-Authored-By: Claude Opus 5.5 --- e2e/tests/journeys.spec.ts | 11 +++++ .../components/ResultsMapView.test.tsx | 11 ++++- .../components/rowContainerGuard.test.ts | 41 +++++++++++++++++++ nextjs-app/components/HomeView.module.css | 33 ++++++++++++++- nextjs-app/components/HomeView.tsx | 12 ++++++ nextjs-app/components/SchoolRow.module.css | 11 ++--- .../components/SecondarySchoolRow.module.css | 11 ++--- 7 files changed, 118 insertions(+), 12 deletions(-) create mode 100644 nextjs-app/__tests__/components/rowContainerGuard.test.ts diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 6d39a70..b7ab141 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -578,6 +578,17 @@ test('a desktop postcode search opens on the map with the list beside it', async await card.click({ position: { x: 6, y: 6 } }); await expect(page.locator('.sc-pin--selected')).toHaveCount(1); await expect(page.locator('.sc-popup')).toContainText(name); + + // And from the keyboard: each row has a "Show … on the map" button that + // appears on focus. + const second = pane.locator('[data-urn]').nth(1); + const secondName = (await second.locator('a').first().innerText()).trim(); + const show = second.getByRole('button', { name: `Show ${secondName} on the map` }); + await show.focus(); + await expect(show).toBeVisible(); + await page.keyboard.press('Enter'); + await expect(show).toHaveAttribute('aria-pressed', 'true'); + await expect(page.locator('.sc-popup')).toContainText(secondName); }); // 402 is the iPhone 17, where the toolbar overflowed (see below). diff --git a/nextjs-app/__tests__/components/ResultsMapView.test.tsx b/nextjs-app/__tests__/components/ResultsMapView.test.tsx index 435edc9..332dbea 100644 --- a/nextjs-app/__tests__/components/ResultsMapView.test.tsx +++ b/nextjs-app/__tests__/components/ResultsMapView.test.tsx @@ -156,10 +156,19 @@ it('builds no list cards on a phone, where the pane is hidden', async () => { it('draws the list view\'s own row beside the map, with the same content', async () => { const { container } = await renderMap(); - const beside = (container.querySelector('[data-urn="2"]') as HTMLElement).textContent; + const beside = container.querySelector('[data-urn="2"] > [class~="row"]')!.textContent; fireEvent.click(screen.getByRole('button', { name: 'List' })); const row = screen.getByRole('link', { name: 'Southmead Primary School' }).closest('[class~="row"]')!; expect(row.parentElement?.className).toMatch(/schoolList/); expect(row.textContent).toBe(beside); }); + +it('lets a keyboard pick a pin from the list, with a real button', async () => { + await renderMap(); + const show = screen.getByRole('button', { name: 'Show Southmead Primary School on the map' }); + expect(show).toHaveAttribute('aria-pressed', 'false'); + fireEvent.click(show); + expect(screen.getByTestId('map')).toHaveAttribute('data-selected', '2'); + expect(show).toHaveAttribute('aria-pressed', 'true'); +}); diff --git a/nextjs-app/__tests__/components/rowContainerGuard.test.ts b/nextjs-app/__tests__/components/rowContainerGuard.test.ts new file mode 100644 index 0000000..b0937f3 --- /dev/null +++ b/nextjs-app/__tests__/components/rowContainerGuard.test.ts @@ -0,0 +1,41 @@ +import fs from 'fs'; +import path from 'path'; + +/* + * SchoolRow and SecondarySchoolRow switch to their narrow layout with a + * container query on a `results` container, not a media query, because the + * same row fills the phone list and the narrow list beside the desktop map. + * Outside a `results` container the query never matches and the row keeps its + * wide layout on a phone, a silent regression rather than an error. + * + * HomeView provides the container on every list it renders the rows into. + * Anything else that starts rendering them must do the same; this fails so + * that the person adding it reads this first. + */ + +const ROOT = path.join(__dirname, '..', '..'); +const DIRS = ['app', 'components', 'lib']; +const ROW_IMPORT = /from\s+['"][^'"]*\/(SchoolRow|SecondarySchoolRow)['"]/; + +function sources(dir: string): string[] { + return fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) return entry.name === 'node_modules' ? [] : sources(full); + return /\.tsx?$/.test(entry.name) ? [full] : []; + }); +} + +it('renders the results rows only where a `results` container is provided', () => { + const importers = DIRS.flatMap((d) => sources(path.join(ROOT, d))) + .filter((file) => ROW_IMPORT.test(fs.readFileSync(file, 'utf8'))) + .map((file) => path.relative(ROOT, file)); + expect(importers).toEqual(['components/HomeView.tsx']); +}); + +it('gives each of HomeView\'s row lists the `results` container', () => { + const css = fs.readFileSync(path.join(ROOT, 'components', 'HomeView.module.css'), 'utf8'); + for (const list of ['.schoolList', '.compactList', '.bottomSheet']) { + const rule = new RegExp(`\\${list}\\s*\\{[^}]*container:\\s*results\\s*/\\s*inline-size`); + expect({ list, provided: rule.test(css) }).toEqual({ list, provided: true }); + } +}); diff --git a/nextjs-app/components/HomeView.module.css b/nextjs-app/components/HomeView.module.css index 11271a8..7ea8c08 100644 --- a/nextjs-app/components/HomeView.module.css +++ b/nextjs-app/components/HomeView.module.css @@ -638,15 +638,46 @@ /* A row in the list beside the map: clicking it picks its pin. */ .mapRow { + position: relative; cursor: pointer; border-radius: 10px; } -.mapRowSelected > * { +.mapRowSelected > :last-child { outline: 2px solid var(--brand); outline-offset: 1px; } +/* Visually hidden until focused, then a pill over the row's top edge. */ +.showOnMap { + position: absolute; + width: 1px; + height: 1px; + overflow: hidden; + clip-path: inset(50%); + white-space: nowrap; +} + +.showOnMap:focus-visible { + top: -0.5rem; + right: 0.75rem; + z-index: 1; + width: auto; + height: auto; + padding: 0.375rem 0.75rem; + overflow: visible; + clip-path: none; + background: var(--brand); + color: var(--brand-on); + border: 0; + border-radius: 999px; + font-family: var(--font-ui); + font-size: var(--step--1); + font-weight: 700; + outline: 2px solid var(--text-primary); + outline-offset: 2px; +} + .sectionHeader { diff --git a/nextjs-app/components/HomeView.tsx b/nextjs-app/components/HomeView.tsx index 1b0d6a5..ceb5ce8 100644 --- a/nextjs-app/components/HomeView.tsx +++ b/nextjs-app/components/HomeView.tsx @@ -834,6 +834,18 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed setSelectedMapSchool(school); }} > + {/* The keyboard's way to pick the pin: hidden until it has + focus, since a pointer just clicks the row. The row + itself cannot be the button, as it holds links and + buttons of its own. */} + {renderRow(school)} ))} diff --git a/nextjs-app/components/SchoolRow.module.css b/nextjs-app/components/SchoolRow.module.css index b68edd1..ce61578 100644 --- a/nextjs-app/components/SchoolRow.module.css +++ b/nextjs-app/components/SchoolRow.module.css @@ -223,12 +223,13 @@ /* * Narrow: content full width, actions in a row beneath. Keyed to the list the * row sits in, not the screen, because the same row fills the phone list and - * the ~400px list beside the map on desktop. HomeView makes both lists a - * `results` container; outside one, the row keeps its wide layout. - * 600px of list is a 632px screen less the page's padding, so phones behave - * as they did under the old max-width: 640px media query. + * the ~430px list beside the map on desktop. HomeView makes its lists a + * `results` container; outside one, the row keeps its wide layout, which is + * why rowContainerGuard.test.ts fails if anything else renders this row. + * 608px is exact: below 769px the page pads 1rem each side, so a 640px screen + * gives a 608px list, matching the old max-width: 640px media query. */ -@container results (max-width: 600px) { +@container results (max-width: 608px) { .row { flex-wrap: wrap; padding: 0.875rem; diff --git a/nextjs-app/components/SecondarySchoolRow.module.css b/nextjs-app/components/SecondarySchoolRow.module.css index 2fb8948..2c82faa 100644 --- a/nextjs-app/components/SecondarySchoolRow.module.css +++ b/nextjs-app/components/SecondarySchoolRow.module.css @@ -235,12 +235,13 @@ /* * Narrow: content full width, actions in a row beneath. Keyed to the list the * row sits in, not the screen, because the same row fills the phone list and - * the ~400px list beside the map on desktop. HomeView makes both lists a - * `results` container; outside one, the row keeps its wide layout. - * 600px of list is a 632px screen less the page's padding, so phones behave - * as they did under the old max-width: 640px media query. + * the ~430px list beside the map on desktop. HomeView makes its lists a + * `results` container; outside one, the row keeps its wide layout, which is + * why rowContainerGuard.test.ts fails if anything else renders this row. + * 608px is exact: below 769px the page pads 1rem each side, so a 640px screen + * gives a 608px list, matching the old max-width: 640px media query. */ -@container results (max-width: 600px) { +@container results (max-width: 608px) { .row { flex-wrap: wrap; padding: 0.875rem;