Compare commits

...
Author SHA1 Message Date
TudorandClaude Fable 5 e5f7f4c959 fix(compare): repair the mobile add-school modal
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m6s
PR Checks / Backend Smoke (pull_request) Successful in 6s
PR Checks / Build Backend (no push) (pull_request) Successful in 10s
PR Checks / Build Frontend (no push) (pull_request) Successful in 48s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 52s
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
2026-07-17 23:02:12 +01:00
TudorandClaude Fable 5 60918da483 fix(compare): give the trend chart its own definite canvas height so the gap note can't squash it
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
2026-07-17 22:58:35 +01:00
tudor 284215fbce Merge pull request 'fix(compare): expert should-fixes S1-S4, S6 (S5/S7 closed by verification)' (#55) from fix/expert-signoff-shouldfixes into main
Stage (build -> staging -> E2E gate) / Build Backend (FastAPI) (push) Successful in 13s
Stage (build -> staging -> E2E gate) / Build Frontend (Next.js) (push) Successful in 53s
Stage (build -> staging -> E2E gate) / Build Pipeline (Meltano + dbt + Airflow) (push) Successful in 14s
Stage (build -> staging -> E2E gate) / Deploy to Staging (push) Successful in 1s
Stage (build -> staging -> E2E gate) / E2E Journeys against Staging (push) Failing after 45s
Reviewed-on: #55
2026-07-17 17:53:16 +00:00
5 changed files with 37 additions and 40 deletions
@@ -1,16 +1,23 @@
/* Chart wrapper: chips (mobile) above, canvas filling the rest of the
parent .chartContainer, whose fixed height drives Chart.js sizing via
maintainAspectRatio: false. */
/* Chart wrapper: chips (mobile) above, then the canvas, then the gap note.
The canvas has its OWN definite height (Chart.js needs one for
maintainAspectRatio: false); the chips and the note flow at their natural
size around it rather than competing with it for a fixed outer height —
so a longer note (e.g. the KS4 gap caption) or a two-row chip legend can
never squash the chart. */
.wrapper {
display: flex;
flex-direction: column;
height: 100%;
}
.canvasBox {
position: relative;
flex: 1 1 auto;
min-height: 0;
height: 380px;
}
@media (max-width: 640px) {
.canvasBox {
height: 280px;
}
}
/* School chips: mobile-only legend + tap-to-focus control. Desktop keeps
+4
View File
@@ -123,6 +123,10 @@
.modal {
width: 100%;
max-width: 100%;
/* Bottom sheet: open at a stable, comfortable height so the empty
"start typing" state isn't a tiny stub and the sheet doesn't jump as
results load (the results list scrolls within). */
min-height: 55vh;
max-height: 95vh;
border-bottom-left-radius: 0;
border-bottom-right-radius: 0;
@@ -2,14 +2,6 @@
padding: 1.5rem;
}
.title {
font-size: 1.5rem;
font-weight: 700;
color: var(--text-primary, #1a1612);
margin-bottom: 1.5rem;
font-family: var(--font-playfair), 'Playfair Display', serif;
}
.warning {
background: var(--accent-gold-bg);
border: 1px solid var(--accent-gold, #c9a227);
@@ -119,12 +111,16 @@
font-size: 1rem;
font-weight: 600;
color: var(--text-primary, #1a1612);
margin-bottom: 0.5rem;
margin-bottom: 0.25rem;
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.resultButton {
flex: 0 0 auto;
}
.schoolMeta {
display: flex;
gap: 1rem;
@@ -159,21 +155,21 @@
padding: 1rem;
}
.title {
font-size: 1.25rem;
}
/* Compact stacked card: name + meta, then a full-width action so the tap
target is obvious and the card doesn't waste vertical space. */
.resultItem {
flex-direction: column;
align-items: stretch;
gap: 0.625rem;
padding: 0.875rem;
}
.addButton {
.resultButton {
width: 100%;
}
.schoolMeta {
flex-direction: column;
gap: 0.25rem;
flex-wrap: wrap;
gap: 0.25rem 1rem;
}
}
+3 -5
View File
@@ -81,10 +81,8 @@ export function SchoolSearchModal({ isOpen, onClose }: SchoolSearchModalProps) {
};
return (
<Modal isOpen={isOpen} onClose={handleClose}>
<Modal isOpen={isOpen} onClose={handleClose} title="Add School to Comparison">
<div className={styles.modalContent}>
<h2 className={styles.title}>Add School to Comparison</h2>
{!canAddMore && (
<div className={styles.warning}>
Maximum 5 schools can be compared. Remove a school to add another.
@@ -129,9 +127,9 @@ export function SchoolSearchModal({ isOpen, onClose }: SchoolSearchModalProps) {
<button
onClick={() => handleAddSchool(school)}
disabled={alreadySelected || !canAddMore}
className={
className={`${styles.resultButton} ${
alreadySelected ? "btn btn-active" : "btn btn-secondary"
}
}`}
>
{alreadySelected ? "✓ Comparing" : "+ Compare"}
</button>
@@ -60,18 +60,10 @@
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. */
/* ComparisonChart owns its own canvas height now (a definite px value per
breakpoint), with the mobile chip legend above and the gap note below it
flowing at natural size. This box therefore only needs to not constrain
that height — no fixed height, or the note would again eat the plot. */
.chartBox {
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;
}
min-height: 0;
}