fix(search): address review on the shared result rows
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m12s
PR Checks / Backend Smoke (pull_request) Successful in 10s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m19s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 21s
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m12s
PR Checks / Backend Smoke (pull_request) Successful in 10s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m19s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 21s
- 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 <school> 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
dff3e210ab
commit
ca4ddd2b12
7 files changed
+118
-12
No files matched your search
@@ -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 card.click({ position: { x: 6, y: 6 } });
|
||||||
await expect(page.locator('.sc-pin--selected')).toHaveCount(1);
|
await expect(page.locator('.sc-pin--selected')).toHaveCount(1);
|
||||||
await expect(page.locator('.sc-popup')).toContainText(name);
|
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).
|
// 402 is the iPhone 17, where the toolbar overflowed (see below).
|
||||||
|
|||||||
@@ -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 () => {
|
it('draws the list view\'s own row beside the map, with the same content', async () => {
|
||||||
const { container } = await renderMap();
|
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' }));
|
fireEvent.click(screen.getByRole('button', { name: 'List' }));
|
||||||
const row = screen.getByRole('link', { name: 'Southmead Primary School' }).closest('[class~="row"]')!;
|
const row = screen.getByRole('link', { name: 'Southmead Primary School' }).closest('[class~="row"]')!;
|
||||||
expect(row.parentElement?.className).toMatch(/schoolList/);
|
expect(row.parentElement?.className).toMatch(/schoolList/);
|
||||||
expect(row.textContent).toBe(beside);
|
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');
|
||||||
|
});
|
||||||
@@ -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 });
|
||||||
|
}
|
||||||
|
});
|
||||||
@@ -638,15 +638,46 @@
|
|||||||
|
|
||||||
/* A row in the list beside the map: clicking it picks its pin. */
|
/* A row in the list beside the map: clicking it picks its pin. */
|
||||||
.mapRow {
|
.mapRow {
|
||||||
|
position: relative;
|
||||||
cursor: pointer;
|
cursor: pointer;
|
||||||
border-radius: 10px;
|
border-radius: 10px;
|
||||||
}
|
}
|
||||||
|
|
||||||
.mapRowSelected > * {
|
.mapRowSelected > :last-child {
|
||||||
outline: 2px solid var(--brand);
|
outline: 2px solid var(--brand);
|
||||||
outline-offset: 1px;
|
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 {
|
.sectionHeader {
|
||||||
|
|||||||
@@ -834,6 +834,18 @@ export function HomeView({ initialSchools, filters, totalSchools, howItWorks, ed
|
|||||||
setSelectedMapSchool(school);
|
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. */}
|
||||||
|
<button
|
||||||
|
type="button"
|
||||||
|
className={styles.showOnMap}
|
||||||
|
aria-pressed={selectedMapSchool?.urn === school.urn}
|
||||||
|
onClick={() => setSelectedMapSchool(school)}
|
||||||
|
>
|
||||||
|
Show {school.school_name} on the map
|
||||||
|
</button>
|
||||||
{renderRow(school)}
|
{renderRow(school)}
|
||||||
</div>
|
</div>
|
||||||
))}
|
))}
|
||||||
|
|||||||
@@ -223,12 +223,13 @@
|
|||||||
/*
|
/*
|
||||||
* Narrow: content full width, actions in a row beneath. Keyed to the list the
|
* 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
|
* 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
|
* the ~430px list beside the map on desktop. HomeView makes its lists a
|
||||||
* `results` container; outside one, the row keeps its wide layout.
|
* `results` container; outside one, the row keeps its wide layout, which is
|
||||||
* 600px of list is a 632px screen less the page's padding, so phones behave
|
* why rowContainerGuard.test.ts fails if anything else renders this row.
|
||||||
* as they did under the old max-width: 640px media query.
|
* 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 {
|
.row {
|
||||||
flex-wrap: wrap;
|
flex-wrap: wrap;
|
||||||
padding: 0.875rem;
|
padding: 0.875rem;
|
||||||
|
|||||||
@@ -235,12 +235,13 @@
|
|||||||
/*
|
/*
|
||||||
* Narrow: content full width, actions in a row beneath. Keyed to the list the
|
* 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
|
* 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
|
* the ~430px list beside the map on desktop. HomeView makes its lists a
|
||||||
* `results` container; outside one, the row keeps its wide layout.
|
* `results` container; outside one, the row keeps its wide layout, which is
|
||||||
* 600px of list is a 632px screen less the page's padding, so phones behave
|
* why rowContainerGuard.test.ts fails if anything else renders this row.
|
||||||
* as they did under the old max-width: 640px media query.
|
* 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 {
|
.row {
|
||||||
flex-wrap: wrap;
|
flex-wrap: wrap;
|
||||||
padding: 0.875rem;
|
padding: 0.875rem;
|
||||||
|
|||||||
Reference in new issue
Block a user