Mobile audit of the compare page. The add-school modal was visibly broken; also folds in the trend-chart height fix that was stranded after PR #55 merged.
Add-school modal (the reported breakage)
SchoolSearchModal rendered <Modal> with no title prop, so Modal's header contained only the close button — which justify-content: space-between pushed to the far left of an otherwise-empty white bar — while the real "Add School to Comparison" title was rendered separately below, inside the content. Result: a stray top-left X over an empty header strip, then a disconnected title.
Pass the title to Modal → header now reads title (left) · close (right), the standard pattern; removed the duplicate in-content <h2>.
The mobile full-width result button targeted a dead .addButton selector (the button is btn btn-secondary, never had that class), so "+ Compare" rendered inconsistently. Gave the button a real module class and full width on mobile.
Tightened the stacked result cards (less wasted vertical space) and gave the bottom sheet a stable min-height: 55vh so its empty "start typing" state isn't a tiny jumping stub.
Trend-chart height (stranded fix, re-applied)
Commit 993a613 from the #55 branch landed after you merged, so it never reached main — the mobile trend chart still squashes to ~178px (below the e2e >220px guard) when a longer gap caption or a two-row chip legend competes with the canvas inside the fixed-height box. Re-applied here: the canvas owns a definite height (280px mobile / 380px desktop) with chips above and the note below flowing at natural size. This is also what unblocks the failing compare chart on mobile e2e test.
Audit — otherwise clean
No horizontal overflow anywhere on the mobile compare page (primary, secondary, all-through); measure-first cards, sticky bar, phase tabs, sections and footer all render correctly at 390px.
Mobile audit of the compare page. The add-school modal was visibly broken; also folds in the trend-chart height fix that was stranded after PR #55 merged.
## Add-school modal (the reported breakage)
`SchoolSearchModal` rendered `<Modal>` with **no `title` prop**, so `Modal`'s header contained only the close button — which `justify-content: space-between` pushed to the far left of an otherwise-empty white bar — while the real "Add School to Comparison" title was rendered separately below, inside the content. Result: a stray top-left X over an empty header strip, then a disconnected title.
- Pass the title to `Modal` → header now reads **title (left) · close (right)**, the standard pattern; removed the duplicate in-content `<h2>`.
- The mobile full-width result button targeted a **dead `.addButton` selector** (the button is `btn btn-secondary`, never had that class), so "+ Compare" rendered inconsistently. Gave the button a real module class and full width on mobile.
- Tightened the stacked result cards (less wasted vertical space) and gave the bottom sheet a stable `min-height: 55vh` so its empty "start typing" state isn't a tiny jumping stub.
## Trend-chart height (stranded fix, re-applied)
Commit `993a613` from the #55 branch landed after you merged, so it never reached main — the mobile trend chart still squashes to ~178px (below the e2e >220px guard) when a longer gap caption or a two-row chip legend competes with the canvas inside the fixed-height box. Re-applied here: the canvas owns a definite height (280px mobile / 380px desktop) with chips above and the note below flowing at natural size. **This is also what unblocks the failing `compare chart on mobile` e2e test.**
## Audit — otherwise clean
No horizontal overflow anywhere on the mobile compare page (primary, secondary, all-through); measure-first cards, sticky bar, phase tabs, sections and footer all render correctly at 390px.
100 frontend tests green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
The chart note (and the 2-row mobile chip legend) lived inside the fixed
360px/420px chartBox and competed with the canvas for it, so a longer
caption — e.g. the new KS4 gap note — shrank the mobile canvas to ~178px
and tripped the e2e >220px guard. The canvas now owns a definite height
(280px mobile / 380px desktop) with chips above and the note flowing
below at natural size, so neither can eat the plot. Verified on staging:
patched canvas renders 280px vs the squashed 178px.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
The 'add school' modal passed no title to Modal, so its header held only
the close button — shoved to the far left by justify-content:space-between
in an otherwise-empty bar — while the real title was rendered separately
inside the content. Pass the title to Modal so the header reads title
(left) + close (right), and drop the duplicate in-content heading.
Also: the mobile full-width result button targeted a dead .addButton
selector (the button never had that class), so it rendered inconsistently
— give the button a real module class and full width on mobile, tighten
the stacked cards, and give the bottom sheet a stable min-height so its
empty state isn't a tiny stub.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
This PR fixes two mobile layout bugs in the school-compare frontend: giving the comparison chart's canvas a definite own height (independent of sibling chips/notes) so a long caption or two-row chip legend can no longer squash the Chart.js plot, and repairing the 'add school' modal by passing a proper title to the shared Modal component and fixing a dead CSS selector on the mobile add button. Both changes are self-contained CSS/JSX layout fixes verified against the existing Modal API and layout structure, with no correctness, security, or deploy issues found.
✅ No issues found.
## 🤖 AI Code Review (Claude Code)
This PR fixes two mobile layout bugs in the school-compare frontend: giving the comparison chart's canvas a definite own height (independent of sibling chips/notes) so a long caption or two-row chip legend can no longer squash the Chart.js plot, and repairing the 'add school' modal by passing a proper title to the shared Modal component and fixing a dead CSS selector on the mobile add button. Both changes are self-contained CSS/JSX layout fixes verified against the existing Modal API and layout structure, with no correctness, security, or deploy issues found.
✅ No issues found.
tudor
merged commit bdd9bef349 into main2026-07-17 22:11:46 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Mobile audit of the compare page. The add-school modal was visibly broken; also folds in the trend-chart height fix that was stranded after PR #55 merged.
Add-school modal (the reported breakage)
SchoolSearchModalrendered<Modal>with notitleprop, soModal's header contained only the close button — whichjustify-content: space-betweenpushed to the far left of an otherwise-empty white bar — while the real "Add School to Comparison" title was rendered separately below, inside the content. Result: a stray top-left X over an empty header strip, then a disconnected title.Modal→ header now reads title (left) · close (right), the standard pattern; removed the duplicate in-content<h2>..addButtonselector (the button isbtn btn-secondary, never had that class), so "+ Compare" rendered inconsistently. Gave the button a real module class and full width on mobile.min-height: 55vhso its empty "start typing" state isn't a tiny jumping stub.Trend-chart height (stranded fix, re-applied)
Commit
993a613from the #55 branch landed after you merged, so it never reached main — the mobile trend chart still squashes to ~178px (below the e2e >220px guard) when a longer gap caption or a two-row chip legend competes with the canvas inside the fixed-height box. Re-applied here: the canvas owns a definite height (280px mobile / 380px desktop) with chips above and the note below flowing at natural size. This is also what unblocks the failingcompare chart on mobilee2e test.Audit — otherwise clean
No horizontal overflow anywhere on the mobile compare page (primary, secondary, all-through); measure-first cards, sticky bar, phase tabs, sections and footer all render correctly at 390px.
100 frontend tests green.
🤖 Generated with Claude Code
https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
🤖 AI Code Review (Claude Code)
This PR fixes two mobile layout bugs in the school-compare frontend: giving the comparison chart's canvas a definite own height (independent of sibling chips/notes) so a long caption or two-row chip legend can no longer squash the Chart.js plot, and repairing the 'add school' modal by passing a proper title to the shared Modal component and fixing a dead CSS selector on the mobile add button. Both changes are self-contained CSS/JSX layout fixes verified against the existing Modal API and layout structure, with no correctness, security, or deploy issues found.
✅ No issues found.