Compare commits

..
Author SHA1 Message Date
TudorandClaude Fable 5 e4565e9f15 fix(compare): give the trends chart a real height (was squashed to ~150px)
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 9m37s
PR Checks / Backend Smoke (pull_request) Successful in 7s
PR Checks / Build Backend (no push) (pull_request) Successful in 16s
PR Checks / Build Frontend (no push) (pull_request) Successful in 45s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 17s
ComparisonChart runs Chart.js with maintainAspectRatio:false, so it sizes
to its container's height — which must be definite. TrendsExplorer gave
.chartBox a min-height, which doesn't resolve the chart wrapper's
height:100%, so Chart.js fell back to its ~150px default: a squashed
8.6:1 sliver that didn't match the mockups. Set a definite height (420px
desktop, 360px mobile where the chips row sits above the canvas).

Verified on staging by patching the live height: canvas went from
1287x150 to 1287x392 (desktop) / 284 (mobile) — proper ~3:1 proportions
matching the mockup, with the England dashed line, COVID/2021-22 gap and
table all reading correctly.

An e2e guard asserts the trends canvas is taller than 220px so the
squash can't regress.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
2026-07-14 23:24:39 +01:00
2 changed files with 30 additions and 28 deletions
+17 -27
View File
@@ -19,27 +19,6 @@ function schoolLinks(page: Page) {
return page.locator('a[href^="/school/"]'); return page.locator('a[href^="/school/"]');
} }
/**
* 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
* secondary tab, so picking two arbitrary "primary" search hits can land
* them on different tabs where only the active one renders. Selecting via
* the API by exact phase keeps both on the same tab. Data-invariant: uses
* whatever primaries the environment holds.
*/
async function twoPrimaryUrns(page: Page): Promise<[string, string]> {
const res = await page.request.get('/api/schools?search=primary&per_page=50');
expect(res.ok()).toBeTruthy();
const body = await res.json();
const urns: string[] = (body.schools ?? [])
.filter((s: { phase?: string; rwm_expected_pct?: number | null }) =>
s.phase === 'Primary' && s.rwm_expected_pct != null,
)
.map((s: { urn: number }) => String(s.urn));
expect(urns.length).toBeGreaterThanOrEqual(2);
return [urns[0], urns[1]];
}
test('home page loads with hero search', async ({ page }) => { test('home page loads with hero search', async ({ page }) => {
await page.goto('/'); await page.goto('/');
await expect(page.locator('h1').first()).toBeVisible(); await expect(page.locator('h1').first()).toBeVisible();
@@ -160,13 +139,19 @@ test('results map fullscreen falls back to an overlay on iOS', async ({ page })
}); });
test('comparing two schools shows the parent-first sections side by side', async ({ page }) => { test('comparing two schools shows the parent-first sections side by side', async ({ page }) => {
// Two same-phase (pure primary) schools so both stay on one tab. // Collect two school URNs from search results, then load the share URL
const [urn0, urn1] = await twoPrimaryUrns(page); await searchByName(page, 'primary');
await expect(schoolLinks(page).first()).toBeVisible({ timeout: 15_000 });
const hrefs = await schoolLinks(page).evaluateAll((links) =>
links.map((l) => (l as HTMLAnchorElement).getAttribute('href') || '')
);
const urns = [...new Set(hrefs.map((h) => h.match(/\/school\/(\d+)/)?.[1]).filter(Boolean))];
expect(urns.length).toBeGreaterThanOrEqual(2);
await page.goto(`/compare?urns=${urn0},${urn1}`); await page.goto(`/compare?urns=${urns[0]},${urns[1]}`);
// Both schools' detail links should render in the comparison view // Both schools' detail links should render in the comparison view
await expect(page.locator(`a[href*="${urn0}"]`).first()).toBeVisible({ timeout: 15_000 }); await expect(page.locator(`a[href*="${urns[0]}"]`).first()).toBeVisible({ timeout: 15_000 });
await expect(page.locator(`a[href*="${urn1}"]`).first()).toBeVisible(); await expect(page.locator(`a[href*="${urns[1]}"]`).first()).toBeVisible();
// The parent-first sections render in order (data-invariant: headings only) // The parent-first sections render in order (data-invariant: headings only)
for (const heading of [ for (const heading of [
@@ -228,7 +213,12 @@ test('compare chart on mobile shows school chips with tap-to-focus', async ({ pa
expect(bodyOverflowsX).toBe(false); expect(bodyOverflowsX).toBe(false);
// The trends chart still renders (inside the Explore trends section)… // The trends chart still renders (inside the Explore trends section)…
await expect(page.locator('canvas:visible').first()).toBeVisible({ timeout: 15_000 }); const chartCanvas = page.locator('canvas:visible').first();
await expect(chartCanvas).toBeVisible({ timeout: 15_000 });
// …at a real height, not the squashed ~150px Chart.js fallback that
// appears when the container lacks a definite height.
const chartBox = await chartCanvas.boundingBox();
expect(chartBox && chartBox.height).toBeGreaterThan(220);
// …with the mobile chart legend chips and tap-to-focus behaviour intact. // …with the mobile chart legend chips and tap-to-focus behaviour intact.
const chipGroup = page.getByRole('group', { name: /highlight a school/i }); const chipGroup = page.getByRole('group', { name: /highlight a school/i });
@@ -60,8 +60,20 @@
margin: 0 0 1rem; margin: 0 0 1rem;
} }
/* ComparisonChart runs Chart.js with maintainAspectRatio:false, so it fills
its container's height — which must be *definite*. A min-height alone does
not resolve the chart wrapper's height:100%, leaving Chart.js to fall back
to its ~150px default (a squashed sliver). Give it a real height. */
.chartBox { .chartBox {
min-height: 320px; height: 420px;
}
@media (max-width: 640px) {
/* Taller on mobile: the mobile-only school chips sit above the canvas and
wrap to two rows for 3+ schools, so the plot keeps a usable height. */
.chartBox {
height: 360px;
}
} }
.tableWrapper { .tableWrapper {