Compare commits

...
Author SHA1 Message Date
TudorandClaude Opus 5 0571d1c0ff fix(web): stop the sheet-open rule stealing .sectionNav's layout
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m12s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m17s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 1m18s
Review catch, and a bad one: the previous commit anchored its insertion on
`padding: 0.5rem 0.75rem;` and closed .sectionNav there. Everything that
followed in the rule — margin-bottom, box-shadow, display: flex, align-items,
gap — was orphaned into .sectionNavSheetOpen, which is only applied while the
mobile jump sheet is open.

So the sticky nav lost its flex layout, spacing and shadow in the closed
state, which is virtually every page view on every school detail page. A
site-wide regression introduced by a fix for one mobile menu.

Redone by anchoring on the complete rule, closing brace included, so nothing
can be orphaned. .sectionNav is now byte-identical to main and the diff is
purely additive; .sectionNavSheetOpen carries the z-index and nothing else.

The staging experiment that validated this fix set nav.style.zIndex = '1100'
with every other declaration intact, so it was always testing this version
rather than the broken one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-22 20:43:58 +01:00
TudorandClaude Opus 5 180d6e9b3e fix(web): lift the jump sheet above the bottom tab bar
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m14s
PR Checks / Backend Smoke (pull_request) Successful in 10s
PR Checks / Build Backend (no push) (pull_request) Successful in 17s
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) Failing after 37s
On mobile the last item in "Jump to section" was painted over by the fixed
bottom tab bar and could not be tapped. Reported as nearby schools missing
from the menu; it was there, underneath the bar.

The sticky nav sets `position: sticky` with `z-index: 10`, which makes it a
stacking context. The sheet's own `z-index: 1600` therefore orders it only
inside that context — against the tab bar (z-index 1000) the nav's 10 is what
counts, so the bar wins. Verified on staging: every menu item returns itself
from elementFromPoint except the last, which returns the tab bar.

Latent rather than new. With five sections the list stopped just above the bar;
"Nearby schools" made six, and the sixth is the first to reach it. Any section
added later would have done the same.

Lifted only while the sheet is open, and only to 1100 — above the bar, below
the comparison toast (2000), the fullscreen map (5000) and the info popover
(9999). The backdrop rises with it, so tapping over the bar now dismisses the
sheet instead of navigating away.

The journey asks what a thumb asks: for each item, whether it is the topmost
element at its own centre. A bounding-box check cannot see this — the item is
in the viewport and the right size, just underneath something. Confirmed to
fail against current staging, naming "Nearby schools", before the fix.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-22 20:35:25 +01:00
tudor 80f405123e Merge pull request 'test(e2e): wait for the carousel's smooth scroll to settle before measuring it' (#153) from fix/nearby-journey-waits-for-smooth-scroll into main
Stage (build -> staging -> E2E gate) / prepare (push) Successful in 1s
Stage (build -> staging -> E2E gate) / Build Backend (FastAPI) (push) Successful in 19s
Stage (build -> staging -> E2E gate) / Build Frontend (Next.js) (push) Successful in 1m25s
Stage (build -> staging -> E2E gate) / Build Pipeline (Meltano + dbt + Airflow) (push) Successful in 13s
Stage (build -> staging -> E2E gate) / Deploy to Staging (push) Successful in 28s
Stage (build -> staging -> E2E gate) / E2E Journeys against Staging (push) Successful in 2m53s
Reviewed-on: #153
2026-09-22 14:55:16 +00:00
TudorandClaude Opus 5 077aca6008 test(e2e): wait for the carousel's smooth scroll to settle before measuring it
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 1m18s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 32s
Second failure of the same assertion, and the first fix addressed a real but
different problem. This is the one that was actually producing the number.

The arrows scroll with `behavior: 'smooth'`. The test polled for "has it moved
at all" — satisfied 50ms in, at 13px of a 1300px journey — then recorded the
offset, clicked, and recorded again. The row was still travelling throughout,
so the delta it measured was the tail of the arrow's animation, not the effect
of the selection. Hence a deterministic 659, roughly half of the 1317 this
school's row scrolls.

Measured against staging rather than reasoned about: the animation runs about
700ms, and the samples are in the helper's comment.

settledScrollLeft waits for two identical readings before trusting one. The app
was never at fault — driving staging by hand, the offset holds at 1317 across
the selection, exactly as intended.

Verified against staging both ways: main's version of this test fails there,
this version passes, along with all three mobile widths.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-22 15:13:32 +01:00
tudor 9dba5ff1ff Merge pull request 'test(e2e): fix the nearby-schools journey clicking a card it scrolled past' (#152) from fix/nearby-scroll-journey-clicks-offscreen-card into main
Stage (build -> staging -> E2E gate) / prepare (push) Successful in 1s
Stage (build -> staging -> E2E gate) / Build Backend (FastAPI) (push) Successful in 20s
Stage (build -> staging -> E2E gate) / Build Frontend (Next.js) (push) Successful in 1m23s
Stage (build -> staging -> E2E gate) / Build Pipeline (Meltano + dbt + Airflow) (push) Successful in 13s
Stage (build -> staging -> E2E gate) / Deploy to Staging (push) Successful in 25s
Stage (build -> staging -> E2E gate) / E2E Journeys against Staging (push) Failing after 2m49s
Reviewed-on: #152
2026-09-22 13:50:25 +00:00
3 changed files with 94 additions and 7 deletions

No files matched your search

+75 -6
View File
@@ -1,4 +1,4 @@
import { test, expect, Page } from '@playwright/test';
import { test, expect, Locator, Page } from '@playwright/test';
/**
* Journey tests for SchoolCompare, run against the staging environment as the
@@ -19,6 +19,31 @@ function schoolLinks(page: Page) {
return page.locator('a[href^="/school/"]');
}
/**
* A scroll offset that has stopped moving.
*
* The carousel arrows scroll with `behavior: 'smooth'`, so a reading taken
* straight after a click lands mid-animation. Measured against staging: the
* animation runs ~700ms, and a poll for "has it moved at all" is satisfied
* 50ms in, at 13px of a 1300px journey. A test that then records an offset,
* does something, and records again is measuring the tail of the arrow's
* animation rather than the effect of whatever it did in between.
*
* Two identical readings in a row is the cheapest sound definition of settled.
*/
async function settledScrollLeft(scroller: Locator): Promise<number> {
let previous = -1;
await expect
.poll(async () => {
const current = await scroller.evaluate((node: HTMLElement) => Math.round(node.scrollLeft));
const settled = current === previous;
previous = current;
return settled;
}, { timeout: 10_000 })
.toBe(true);
return previous;
}
/**
* Two URNs guaranteed to be pure-primary (same phase). The compare page's
* phase tabs split all-through schools (which carry KS4 data) onto the
@@ -2776,9 +2801,7 @@ test('nearby schools link on to other schools and into compare', async ({ page }
await expect(back).toBeDisabled();
await forward.click();
await expect
.poll(() => scroller.evaluate((node: HTMLElement) => node.scrollLeft))
.toBeGreaterThan(8);
expect(await settledScrollLeft(scroller)).toBeGreaterThan(8);
await expect(back).toBeEnabled();
}
@@ -2794,15 +2817,61 @@ test('nearby schools link on to other schools and into compare', async ({ page }
//
// A few pixels of snap or sub-pixel adjustment are fine; a reset to the
// start is not, which is the whole point of the check.
const offsetBefore = await scroller.evaluate((node: HTMLElement) => node.scrollLeft);
const offsetBefore = await settledScrollLeft(scroller);
await section.getByRole('button', { name: /Add to compare/ }).last().click();
await expect(
section.getByRole('button', { name: /Added to compare/ }).first(),
).toBeVisible();
const offsetAfter = await scroller.evaluate((node: HTMLElement) => node.scrollLeft);
const offsetAfter = await settledScrollLeft(scroller);
expect(Math.abs(offsetAfter - offsetBefore)).toBeLessThanOrEqual(8);
});
/**
* Every section in the mobile jump sheet can actually be reached.
*
* The sheet is a fixed bottom sheet, and the app has a fixed bottom tab bar.
* `position: sticky` with a z-index on the sticky nav makes it a stacking
* context, so the sheet's own z-index orders it only within that context —
* against the tab bar, the nav's value is what counts. The last item in the
* sheet was therefore painted over and untappable as soon as the list grew
* long enough to reach the bar, which adding "Nearby schools" is what did.
*
* Bounding boxes are not enough to catch this: the item is in the viewport and
* the right size, it is simply underneath something. So this asks the question
* a thumb asks — what is on top at this point.
*/
test('every section in the mobile jump sheet is tappable, not under the tab bar', async ({ page }) => {
await page.setViewportSize({ width: 390, height: 844 });
await searchByName(page, 'Primary');
await schoolLinks(page).first().click();
await page.waitForURL(/\/school\//);
// Scroll down so the sticky nav is docked and the sheet has somewhere to open.
await page.evaluate(() => window.scrollTo({ top: 1200 }));
// Two controls carry aria-haspopup: the mobile "Section" button and the
// desktop "All" one, which is display:none here but still in the DOM.
await page.locator('[aria-haspopup="menu"]:visible').click();
const sheet = page.locator('[role="menu"]');
await expect(sheet).toBeVisible();
const covered = await sheet.evaluate((panel: HTMLElement) =>
Array.from(panel.querySelectorAll('[role="menuitem"]'))
.map((el) => {
const box = el.getBoundingClientRect();
const hit = document.elementFromPoint(
Math.round(box.left + box.width / 2),
Math.round(box.top + box.height / 2),
);
return { label: (el as HTMLElement).innerText.trim().replace(/\s+/g, ' '), reachable: !!(hit && hit.closest('[role="menuitem"]')) };
})
.filter((item) => !item.reachable)
.map((item) => item.label),
);
expect(covered).toEqual([]);
});
/**
* The nearby-schools section at MOBILE.md's three reference widths.
*
@@ -283,6 +283,21 @@
}
/* `position: sticky` with a z-index makes .sectionNav a stacking context, so
the jump sheet's own z-index only orders it INSIDE that context. Against the
fixed bottom tab bar (Navigation.module.css, z-index 1000) what counts is
.sectionNav's 10 — which is why the sheet's last item was painted over, and
untappable, once the list grew long enough to reach the bar.
Lifted only while the sheet is open, and only to 1100: above the bar, below
the comparison toast (2000), the fullscreen map (5000) and the info popover
(9999). This rule carries the z-index and nothing else; every other
declaration belongs to .sectionNav in both states. */
.sectionNavSheetOpen {
z-index: 1100;
}
.sectionNavBack {
flex: none;
display: inline-flex;
@@ -344,7 +344,10 @@ export function SchoolDetailShell({
</header>
{/* Sticky Section Navigation — docks under the global header */}
<nav className={styles.sectionNav} aria-label="Page sections">
<nav
className={`${styles.sectionNav}${sectionsOpen ? ` ${styles.sectionNavSheetOpen}` : ''}`}
aria-label="Page sections"
>
<button onClick={scrollToTop} className={styles.sectionNavBack} aria-label="Back to top">
<span aria-hidden="true">↑</span>
<span className={styles.sectionNavBackLabel}>Top</span>