Compare commits

...
Author SHA1 Message Date
TudorandClaude Fable 5 2fd997bfe6 fix(compare): keep the add-school sheet above the on-screen keyboard
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m6s
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 47s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 1m31s
The bottom-sheet overlay was position:fixed inset:0, sized to the LAYOUT
viewport, so when the mobile keyboard opened it kept full height: the
sheet (anchored to the overlay's bottom) and the lower half of the dim
backdrop sat behind the keyboard, hiding the search input and results.

Track window.visualViewport (height + offsetTop) while the modal is open
and pin the overlay to the visible region, so backdrop and sheet stay
above the keyboard. Size the sheet against the overlay (max-height:100%,
min-height:min(55vh,100%)) instead of vh, and drop the results list's own
inner scroll on mobile so the sheet is a single scroll area.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146VHeLAWjDVE2B5uU67jCB
2026-07-18 08:00:31 +01:00
tudor bdd9bef349 Merge pull request 'fix(compare): repair mobile add-school modal + re-apply stranded chart-height fix' (#56) from fix/mobile-compare-polish 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 51s
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 1s
Stage (build -> staging -> E2E gate) / E2E Journeys against Staging (push) Successful in 41s
Reviewed-on: #56
2026-07-17 22:11:45 +00:00
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
6 changed files with 77 additions and 41 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
+7 -1
View File
@@ -123,7 +123,13 @@
.modal {
width: 100%;
max-width: 100%;
max-height: 95vh;
/* Bottom sheet sized against the overlay (which tracks the visual
viewport), NOT vh: when the keyboard is open the overlay is short, so
max-height:100% keeps the whole sheet — input and results — above the
keyboard. min-height gives a comfortable default without a tiny stub,
but is capped at 100% so it never exceeds the visible area. */
min-height: min(55vh, 100%);
max-height: 100%;
border-bottom-left-radius: 0;
border-bottom-right-radius: 0;
animation: slideUp 0.3s ease;
+30 -2
View File
@@ -5,7 +5,7 @@
'use client';
import { useEffect, useCallback } from 'react';
import { useEffect, useCallback, useRef } from 'react';
import { createPortal } from 'react-dom';
import styles from './Modal.module.css';
@@ -18,6 +18,8 @@ interface ModalProps {
}
export function Modal({ isOpen, onClose, children, title, size = 'medium' }: ModalProps) {
const overlayRef = useRef<HTMLDivElement>(null);
const handleEscape = useCallback((e: KeyboardEvent) => {
if (e.key === 'Escape') {
onClose();
@@ -39,6 +41,32 @@ export function Modal({ isOpen, onClose, children, title, size = 'medium' }: Mod
};
}, [isOpen, handleEscape]);
// Pin the overlay to the VISUAL viewport, not the layout viewport. On mobile
// the on-screen keyboard shrinks the visual viewport but not the layout one,
// so a `position: fixed; inset: 0` overlay keeps full height — leaving the
// bottom-anchored sheet (and the dim backdrop's lower half) hidden behind
// the keyboard. Tracking visualViewport.height/offsetTop keeps the whole
// overlay — backdrop and sheet — inside the visible area, above the keyboard.
useEffect(() => {
if (!isOpen) return;
const vv = typeof window !== 'undefined' ? window.visualViewport : null;
const el = overlayRef.current;
if (!vv || !el) return;
const sync = () => {
el.style.top = `${vv.offsetTop}px`;
el.style.height = `${vv.height}px`;
el.style.bottom = 'auto';
};
sync();
vv.addEventListener('resize', sync);
vv.addEventListener('scroll', sync);
return () => {
vv.removeEventListener('resize', sync);
vv.removeEventListener('scroll', sync);
};
}, [isOpen]);
if (!isOpen || typeof window === 'undefined') return null;
const handleOverlayClick = (e: React.MouseEvent) => {
@@ -48,7 +76,7 @@ export function Modal({ isOpen, onClose, children, title, size = 'medium' }: Mod
};
return createPortal(
<div className={styles.overlay} onClick={handleOverlayClick}>
<div ref={overlayRef} className={styles.overlay} onClick={handleOverlayClick}>
<div className={`${styles.modal} ${styles[size]}`}>
<div className={styles.header}>
{title && <h2 className={styles.title}>{title}</h2>}
@@ -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,30 @@
padding: 1rem;
}
.title {
font-size: 1.25rem;
/* One scroll container on mobile: the modal content itself scrolls, so the
results list must not add its own inner scroll (double scrollbars, and
the input would be trapped above a short 400px window when the keyboard
shrinks the sheet). */
.results {
max-height: none;
overflow: visible;
}
/* 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;
}