Compare commits

..
Author SHA1 Message Date
TudorandClaude Opus 5 dbb74d9b60 fix(places): align the measure column's heading with its values
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 7s
PR Checks / Build Backend (no push) (pull_request) Successful in 11s
PR Checks / Build Frontend (no push) (pull_request) Successful in 44s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 14s
The heading sat on the right edge of the column and every value on the left.
A specificity collision, not a layout problem: the two were aligned by
different selectors and only one of them won.

  .table td            (0,1,1)  text-align: left    <- won for the value
  .num                 (0,1,0)  text-align: right   <- lost
  .table th:last-child (0,2,1)  text-align: right   <- won for the heading

The heading and the value cell now share one class and one rule, so they
cannot drift apart again whatever else changes around them.

The column also stretched to half the table. It now hugs its content with
width:1% and nowrap, so the school name takes the remaining width — which is
what made the gap read as misalignment on a wide screen, and what crowded the
name column on a narrow one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-21 22:15:02 +01:00
8 changed files with 44 additions and 205 deletions

No files matched your search

-7
View File
@@ -1232,13 +1232,6 @@ async def get_place(request: Request, kind: str, slug: str,
"place": {"kind": place.kind, "slug": place.slug, "name": place.name, "place": {"kind": place.kind, "slug": place.slug, "name": place.name,
"count": len(place.urns), "count": len(place.urns),
"parent_authority": place.parent_authority, "parent_authority": place.parent_authority,
# Every authority the place meaningfully sits in. SW19 is
# mostly Merton but partly Wandsworth; naming one asserts
# something false.
"authorities": [
{"name": name, "slug": _slugify(name), "count": n}
for name, n in place.authorities
],
# Only phases that clear the threshold, so the page links # Only phases that clear the threshold, so the page links
# variants that exist rather than 404s. # variants that exist rather than 404s.
"phases": [ph for ph in ("primary", "secondary") "phases": [ph for ph in ("primary", "secondary")
-45
View File
@@ -30,12 +30,6 @@ class Place:
name: str name: str
urns: tuple[int, ...] urns: tuple[int, ...]
parent_authority: str | None # authority NAME, for the 301 target parent_authority: str | None # authority NAME, for the 301 target
# Every authority the place meaningfully sits in, largest first. A quarter
# of outcodes and a third of towns straddle a boundary — SW19 is mostly
# Merton but partly Wandsworth — so naming only one asserts something
# false. parent_authority stays single because a redirect needs one
# target; this is what the page shows.
authorities: tuple[tuple[str, int], ...] = ()
# URNs per phase, so the per-phase threshold can be applied without # URNs per phase, so the per-phase threshold can be applied without
# re-querying. A place with 30 primaries and 2 secondaries publishes a # re-querying. A place with 30 primaries and 2 secondaries publishes a
# primary variant and no secondary one. # primary variant and no secondary one.
@@ -77,42 +71,6 @@ def _phase_urns(group, publishable: set[int]) -> dict[str, tuple[int, ...]]:
return out return out
# A place is described by an authority when it holds at least a tenth of the
# schools, and at least two. GIAS carries occasional postcode errors — EN6
# lists two Shropshire schools among fourteen in Hertfordshire — and a bare
# "any authority present" rule would print those as though they were real.
_AUTHORITY_MIN_SHARE = 0.10
_AUTHORITY_MIN_SCHOOLS = 2
_AUTHORITY_MAX_SHOWN = 3
def _authorities(group) -> tuple[tuple[str, int], ...]:
"""Authorities this place meaningfully sits in, largest first."""
from backend.app import EXCLUDED_FILTER_VALUES
if "local_authority" not in group.columns:
return ()
counts = group["local_authority"].dropna().value_counts()
total = int(counts.sum())
if not total:
return ()
kept = [
(str(name), int(n)) for name, n in counts.items()
if str(name) not in EXCLUDED_FILTER_VALUES
and n >= _AUTHORITY_MIN_SCHOOLS
and n / total >= _AUTHORITY_MIN_SHARE
]
# A place too small or too fragmented for the share rule still names its
# largest authority, or the page would say nothing about where it is.
if not kept:
for name, n in counts.items():
if str(name) not in EXCLUDED_FILTER_VALUES:
return ((str(name), int(n)),)
return ()
return tuple(kept[:_AUTHORITY_MAX_SHOWN])
def _parent_authority(group) -> str | None: def _parent_authority(group) -> str | None:
"""The most common authority in a group — the useful 301 target. """The most common authority in a group — the useful 301 target.
@@ -146,7 +104,6 @@ def _group(df, column: str, kind: str, publishable: set[int]) -> dict[str, Place
place = Place( place = Place(
kind=kind, slug=slug, name=name, urns=urns, kind=kind, slug=slug, name=name, urns=urns,
parent_authority=_parent_authority(group) if kind == "town" else None, parent_authority=_parent_authority(group) if kind == "town" else None,
authorities=() if kind == "authority" else _authorities(group),
phase_urns=_phase_urns(group, publishable), phase_urns=_phase_urns(group, publishable),
) )
out[place.key] = place out[place.key] = place
@@ -181,7 +138,6 @@ def _outcode_places(df, publishable: set[int]) -> dict[str, Place]:
continue continue
place = Place(kind="outcode", slug=str(oc).lower(), name=str(oc), place = Place(kind="outcode", slug=str(oc).lower(), name=str(oc),
urns=urns, parent_authority=_parent_authority(group), urns=urns, parent_authority=_parent_authority(group),
authorities=_authorities(group),
phase_urns=_phase_urns(group, publishable)) phase_urns=_phase_urns(group, publishable))
out[place.key] = place out[place.key] = place
return out return out
@@ -225,7 +181,6 @@ def _locality_places(df, publishable: set[int],
continue continue
place = Place(kind="locality", slug=slug, name=name, urns=urns, place = Place(kind="locality", slug=slug, name=name, urns=urns,
parent_authority=_parent_authority(group), parent_authority=_parent_authority(group),
authorities=_authorities(group),
phase_urns=_phase_urns(group, publishable)) phase_urns=_phase_urns(group, publishable))
out[place.key] = place out[place.key] = place
return out return out
-61
View File
@@ -229,64 +229,3 @@ def test_no_curated_locality_names_a_london_borough():
f"these are boroughs, not districts: {sorted(named)} - they already " f"these are boroughs, not districts: {sorted(named)} - they already "
"have an authority page covering every school" "have an authority page covering every school"
) )
def test_a_place_names_every_authority_it_straddles():
"""SW19 is mostly Merton but partly Wandsworth.
A quarter of viable outcodes and a third of viable towns cross an
authority boundary, so naming only the largest asserts something false.
"""
rows = (_town(26, "London", "Merton", start=300000)
+ _town(7, "London", "Wandsworth", start=400000))
for r in rows:
r["postcode"] = "SW19 1AA"
reg = build_place_registry(_df(rows))
names = [n for n, _ in reg["outcode:sw19"].authorities]
assert names == ["Merton", "Wandsworth"] # largest first
assert dict(reg["outcode:sw19"].authorities)["Wandsworth"] == 7
def test_the_redirect_target_stays_a_single_authority():
# parent_authority and authorities do different jobs: a 301 needs one
# target, the page needs the truth.
rows = (_town(26, "London", "Merton", start=300000)
+ _town(7, "London", "Wandsworth", start=400000))
for r in rows:
r["postcode"] = "SW19 1AA"
reg = build_place_registry(_df(rows))
assert reg["outcode:sw19"].parent_authority == "Merton"
def test_a_stray_authority_below_the_share_threshold_is_not_named():
# GIAS carries postcode errors — EN6 lists two Shropshire schools among
# fourteen in Hertfordshire. Printing those as though real would be worse
# than omitting them.
rows = (_town(30, "Barnet", "Hertfordshire", start=300000)
+ _town(1, "Barnet", "Shropshire", start=400000))
for r in rows:
r["postcode"] = "EN6 1AA"
reg = build_place_registry(_df(rows))
assert [n for n, _ in reg["outcode:en6"].authorities] == ["Hertfordshire"]
def test_a_sentinel_authority_is_never_named():
rows = (_town(20, "London", "Merton", start=300000)
+ _town(6, "London", "Does not apply", start=400000))
for r in rows:
r["postcode"] = "SW19 1AA"
reg = build_place_registry(_df(rows))
assert [n for n, _ in reg["outcode:sw19"].authorities] == ["Merton"]
def test_a_place_always_names_at_least_one_authority():
# Even when every authority is below the share threshold, the page has to
# say where the place is.
rows = []
for i, la in enumerate(["A", "B", "C", "D", "E", "F", "G"]):
rows += _town(1, "Fragmented", la, start=300000 + i * 100)
reg = build_place_registry(_df(rows))
place = reg.get("town:fragmented")
assert place is not None
assert len(place.authorities) == 1
-26
View File
@@ -1885,29 +1885,3 @@ test('no page title repeats the brand', async ({ page }) => {
expect(brands, `${path} repeats the brand: ${title}`).toBeLessThanOrEqual(1); expect(brands, `${path} repeats the brand: ${title}`).toBeLessThanOrEqual(1);
} }
}); });
test('a place straddling a boundary names every authority it sits in', async ({ page }) => {
// A quarter of outcodes and a third of towns cross an authority boundary —
// SW19 is mostly Merton but partly Wandsworth. Naming only the largest
// asserts something false about the place.
const { places } = await (await page.request.get('/api/places')).json();
const outcode = places.find((p: { kind: string }) => p.kind === 'outcode');
expect(outcode).toBeTruthy();
// Find any place the registry reports as straddling.
let straddling: { kind: string; slug: string } | null = null;
for (const p of places.filter((p: { kind: string }) => p.kind === 'outcode').slice(0, 40)) {
const d = await (await page.request.get(`/api/places/outcode/${p.slug}`)).json();
if ((d.place.authorities ?? []).length > 1) { straddling = p; break; }
}
test.skip(!straddling, 'no straddling outcode found in the sample');
const detail = await (await page.request.get(
`/api/places/outcode/${straddling!.slug}`)).json();
await page.goto(`/schools/near/${straddling!.slug}`);
for (const a of detail.place.authorities) {
await expect(page.locator(`a[href="/schools/authority/${a.slug}"]`).first())
.toBeVisible();
}
});
@@ -169,14 +169,10 @@ describe('PlaceView presentation', () => {
}); });
}); });
describe('PlaceView authorities', () => { describe('PlaceView table alignment', () => {
const straddling: PlaceDetail = { const aligned: PlaceDetail = {
place: { kind: 'outcode', slug: 'sw19', name: 'SW19', count: 33, place: { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 2,
parent_authority: 'Merton', phases: ['primary'], parent_authority: 'Essex', phases: ['primary'] },
authorities: [
{ name: 'Merton', slug: 'merton', count: 26 },
{ name: 'Wandsworth', slug: 'wandsworth', count: 7 },
] },
schools: [ schools: [
{ urn: 1, school_name: 'Alpha Primary', phase: 'Primary', { urn: 1, school_name: 'Alpha Primary', phase: 'Primary',
rwm_expected_pct: 82, attainment_8_score: null } as never, rwm_expected_pct: 82, attainment_8_score: null } as never,
@@ -184,31 +180,22 @@ describe('PlaceView authorities', () => {
averages: { rwm_expected_pct: 63, attainment_8_score: null }, averages: { rwm_expected_pct: 63, attainment_8_score: null },
}; };
it('names every authority the place straddles, not just the largest', () => { it('aligns the measure heading and its values with the same class', () => {
// SW19 is mostly Merton but partly Wandsworth. Naming one asserts // They were aligned by two different selectors whose specificity did not
// something false about a quarter of outcodes. // match: `.table th:last-child` (0,2,1) won and went right, while `.num`
render(<PlaceView detail={straddling} englandAverage={61} neighbours={[]} />); // (0,1,0) lost to `.table td` (0,1,1) and stayed left. Sharing one class
expect(screen.getByRole('link', { name: 'Merton' })) // is what makes them impossible to drift apart.
.toHaveAttribute('href', '/schools/authority/merton'); const { container } = render(<PlaceView detail={aligned} englandAverage={61}
expect(screen.getByRole('link', { name: 'Wandsworth' })) neighbours={[]} />);
.toHaveAttribute('href', '/schools/authority/wandsworth'); const th = container.querySelectorAll('th')[1];
const td = container.querySelectorAll('tbody td')[1];
expect(th.className).toBeTruthy();
expect(td.className).toBe(th.className);
}); });
it('joins them readably rather than as a bare list', () => { it('leaves the school-name column unclassed so it takes the spare width', () => {
// Asserted on the summary line's whole text: a loose /and/ matcher also const { container } = render(<PlaceView detail={aligned} englandAverage={61}
// hits "Wandsworth". neighbours={[]} />);
const { container } = render(<PlaceView detail={straddling} expect(container.querySelectorAll('th')[0].className).toBe('');
englandAverage={61} neighbours={[]} />);
const summary = container.querySelector('header p');
expect(summary?.textContent).toContain('Merton and Wandsworth');
});
it('falls back to the single parent when the field is absent', () => {
// A cached API response predating the authorities field must not blank
// the line entirely.
const legacy = { ...straddling,
place: { ...straddling.place, authorities: undefined } };
render(<PlaceView detail={legacy} englandAverage={61} neighbours={[]} />);
expect(screen.getByRole('link', { name: 'Merton' })).toBeInTheDocument();
}); });
}); });
@@ -144,10 +144,24 @@
border-bottom: none; border-bottom: none;
} }
.table th:last-child, /*
.num { * Header and value share one class and one rule, so they cannot drift apart.
*
* The first cut aligned them with two different selectors: `.table th:last-child`
* at (0,2,1) beat the element rule and went right, while `.num` at (0,1,0) lost
* to `.table td` at (0,1,1) and stayed left. The heading and its numbers sat on
* opposite edges of the column.
*
* width:1% with nowrap makes the measure column hug its content so the school
* name takes the remaining width — without it the two columns split evenly and
* the gap between heading and value reads as misalignment on a wide screen.
*/
.table th.num,
.table td.num {
text-align: right; text-align: right;
font-variant-numeric: tabular-nums; font-variant-numeric: tabular-nums;
width: 1%;
white-space: nowrap;
} }
/* The measure is spelled out; the tooltip carries the definition. */ /* The measure is spelled out; the tooltip carries the definition. */
+8 -20
View File
@@ -71,7 +71,9 @@ function SchoolTable({ schools, phase }: { schools: School[]; phase: PhaseKey })
<thead> <thead>
<tr> <tr>
<th scope="col">School</th> <th scope="col">School</th>
<th scope="col"> {/* Same class as the value cell below: one rule aligns both, so
they cannot drift apart. */}
<th scope="col" className={styles.num}>
<abbr className={styles.metricHead} title={metric.hint}> <abbr className={styles.metricHead} title={metric.hint}>
{metric.heading} {metric.heading}
</abbr> </abbr>
@@ -104,13 +106,6 @@ function SchoolTable({ schools, phase }: { schools: School[]; phase: PhaseKey })
export function PlaceView({ detail, phase, englandAverage, neighbours }: Props) { export function PlaceView({ detail, phase, englandAverage, neighbours }: Props) {
const { place, schools, averages } = detail; const { place, schools, averages } = detail;
// Fall back to the single parent when the API predates the authorities
// field, so a stale cache never blanks the line entirely.
const authorities = place.authorities?.length
? place.authorities
: place.parent_authority
? [{ name: place.parent_authority, slug: authoritySlug(place.parent_authority), count: 0 }]
: [];
const local = averages[METRICS[phase ?? 'primary'].key]; const local = averages[METRICS[phase ?? 'primary'].key];
const phaseWord = phase === 'secondary' ? 'Secondary schools' const phaseWord = phase === 'secondary' ? 'Secondary schools'
: phase === 'primary' ? 'Primary schools' : 'Schools'; : phase === 'primary' ? 'Primary schools' : 'Schools';
@@ -168,20 +163,13 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
<h1>{phaseWord} in {place.name}</h1> <h1>{phaseWord} in {place.name}</h1>
<p className={styles.summary}> <p className={styles.summary}>
{place.count} schools {place.count} schools
{authorities.length > 0 && ( {place.parent_authority && (
<> <>
{' · '} {' · '}
{/* Every authority, not just the largest. A quarter of outcodes <Link href={`/schools/authority/${authoritySlug(place.parent_authority)}`}
and a third of towns cross a boundary: SW19 is mostly Merton className={styles.inlineLink}>
but partly Wandsworth, and naming one asserts otherwise. */} {place.parent_authority}
{authorities.map((a, i) => ( </Link>
<span key={a.slug}>
{i > 0 && (i === authorities.length - 1 ? ' and ' : ', ')}
<Link href={`/schools/authority/${a.slug}`} className={styles.inlineLink}>
{a.name}
</Link>
</span>
))}
</> </>
)} )}
</p> </p>
+1 -12
View File
@@ -18,19 +18,8 @@ export interface PlaceSummary {
phases?: string[]; phases?: string[];
} }
export interface PlaceAuthority {
name: string;
slug: string;
count: number;
}
export interface PlaceDetail { export interface PlaceDetail {
place: PlaceSummary & { place: PlaceSummary & { parent_authority: string | null };
parent_authority: string | null;
/** Every authority the place meaningfully sits in, largest first. SW19 is
* mostly Merton but partly Wandsworth. */
authorities?: PlaceAuthority[];
};
schools: School[]; schools: School[];
averages: { averages: {
rwm_expected_pct: number | null; rwm_expected_pct: number | null;