diff --git a/nextjs-app/__tests__/components/FilterSheetPending.test.tsx b/nextjs-app/__tests__/components/FilterSheetPending.test.tsx new file mode 100644 index 0000000..19dd2e7 --- /dev/null +++ b/nextjs-app/__tests__/components/FilterSheetPending.test.tsx @@ -0,0 +1,80 @@ +import { act, fireEvent, render, screen, within } from '@testing-library/react'; +import { FilterBar } from '@/components/FilterBar'; + +/* + * While a filter change is navigating, the sheet's controls stay enabled: a + * control disabled under the user's focus drops it to , and a keyboard or + * screen-reader user is thrown out of the sheet after every change. A second + * change made before the first lands must build on the first, not on the URL + * useSearchParams still reports. + */ + +// Every transition stays pending, as a slow server render would. +jest.mock('react', () => ({ + ...jest.requireActual('react'), + useTransition: () => [true, (fn: () => void) => fn()], +})); + +const params = new URLSearchParams('postcode=SW196AR&radius=1'); +const push = jest.fn(); +jest.mock('next/navigation', () => ({ + useSearchParams: () => params, + usePathname: () => '/', + useRouter: () => ({ push, replace: jest.fn(), prefetch: jest.fn() }), +})); +jest.mock('@/lib/analytics', () => ({ track: jest.fn() })); + +const filters = { + local_authorities: ['Wandsworth'], school_types: ['Community school'], years: [], + phases: ['Primary', 'Secondary'], genders: [], admissions_policies: [], +}; + +const pushedParams = () => new URLSearchParams(push.mock.calls.at(-1)![0].split('?')[1]); + +const openSheet = () => { + fireEvent.click(screen.getByRole('button', { name: /^Filters/ })); + return screen.getByRole('dialog', { name: 'Filters' }); +}; + +beforeEach(() => push.mockClear()); + +it('keeps the sheet usable, and says it is busy, while a change lands', () => { + render(); + const sheet = openSheet(); + for (const name of ['Phase', 'School type', 'Local authority']) { + expect(within(sheet).getByRole('combobox', { name })).toBeEnabled(); + } + expect(within(sheet).getByRole('radio', { name: 'Within 3 miles' })).toBeEnabled(); + expect(sheet.querySelector('[aria-busy="true"]')).not.toBeNull(); +}); + +it('builds a second change on the first, not on the URL it has not reached', () => { + render(); + const sheet = openSheet(); + fireEvent.change(within(sheet).getByRole('combobox', { name: 'Phase' }), { target: { value: 'primary' } }); + fireEvent.change(within(sheet).getByRole('combobox', { name: 'School type' }), + { target: { value: 'Community school' } }); + const next = pushedParams(); + expect(next.get('phase')).toBe('primary'); + expect(next.get('school_type')).toBe('Community school'); + expect(next.get('postcode')).toBe('SW196AR'); +}); + +describe('a screen that widens past phone width', () => { + let listeners: ((e: { matches: boolean }) => void)[] = []; + beforeEach(() => { + listeners = []; + window.matchMedia = jest.fn().mockImplementation((query: string) => ({ + matches: true, media: query, + addEventListener: (_: string, l: (e: { matches: boolean }) => void) => listeners.push(l), + removeEventListener: jest.fn(), + })); + }); + + it('closes the sheet, leaving the desktop row as the only filters', () => { + render(); + openSheet(); + act(() => listeners.forEach((l) => l({ matches: false }))); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); +}); diff --git a/nextjs-app/components/FilterBar.tsx b/nextjs-app/components/FilterBar.tsx index f10ab28..036b7fa 100644 --- a/nextjs-app/components/FilterBar.tsx +++ b/nextjs-app/components/FilterBar.tsx @@ -247,9 +247,21 @@ export function FilterBar({ return () => document.removeEventListener("keydown", handleKeyDown); }, []); + /* + * Where the navigation in flight is headed. useSearchParams only catches up + * once a navigation lands, so a second change made before then (the phone + * sheet stays usable while one lands) would otherwise be built on the old + * URL and undo the first. Dropped once nothing is pending, when + * searchParams is current again. + */ + const pendingQueryRef = useRef(null); + useEffect(() => { + if (!isPending) pendingQueryRef.current = null; + }, [isPending]); + const updateURL = useCallback( (updates: Record) => { - const params = new URLSearchParams(searchParams); + const params = new URLSearchParams(pendingQueryRef.current ?? searchParams); Object.entries(updates).forEach(([key, value]) => { if (value && value !== "") { @@ -260,6 +272,7 @@ export function FilterBar({ }); params.delete("page"); + pendingQueryRef.current = params.toString(); startTransition(() => { router.push(`${pathname}?${params.toString()}`); @@ -324,6 +337,7 @@ export function FilterBar({ const handleClearFilters = () => { setOmniValue(""); + pendingQueryRef.current = ""; startTransition(() => { router.push(pathname); }); @@ -367,6 +381,18 @@ export function FilterBar({ * "More filters" opened did not contain either of them. */ const [sheetOpen, setSheetOpen] = useState(false); + // The sheet is phone furniture: if the screen widens past phone width while + // it is open (rotation, a resized window), close it, or its selects and the + // desktop row's would both be showing. + useEffect(() => { + if (!sheetOpen || typeof window.matchMedia !== "function") return; + const phone = window.matchMedia("(max-width: 640px)"); + const onChange = (e: { matches: boolean }) => { + if (!e.matches) setSheetOpen(false); + }; + phone.addEventListener("change", onChange); + return () => phone.removeEventListener("change", onChange); + }, [sheetOpen]); const values: Record = { phase: currentPhase, school_type: currentType, @@ -401,6 +427,8 @@ export function FilterBar({ * Each select is built here once and drawn twice: as a pill in the desktop * row or panel, and as a full-width labelled field in the phone sheet. Only * one copy is ever showing; the sheet's is mounted only while it is open. + * The sheet's stay enabled while a change lands: disabling the control + * under the user's focus would drop focus to , out of the dialog. */ type Look = "pill" | "panel" | "sheet"; const selectClass = (look: Look, value: string) => @@ -413,7 +441,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("phase", e.target.value)} className={selectClass(look, currentPhase)} aria-label="Phase" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {phaseOptions.map((p) => ( @@ -432,7 +460,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("school_type", e.target.value)} className={selectClass(look, currentType)} aria-label="School type" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {typeOptions.map((type) => ( @@ -451,7 +479,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("local_authority", e.target.value)} className={selectClass(look, currentLA)} aria-label="Local authority" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {laOptions.map((la) => ( @@ -470,7 +498,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("gender", e.target.value)} className={selectClass(look, currentGender)} aria-label="Gender" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {genderOptions.map((g) => ( @@ -489,7 +517,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("has_sixth_form", e.target.value)} className={selectClass(look, currentHasSixthForm)} aria-label="Sixth form" - disabled={isPending} + disabled={isPending && look !== "sheet"} > @@ -505,7 +533,7 @@ export function FilterBar({ onChange={(e) => handleFilterChange("admissions_policy", e.target.value)} className={selectClass(look, currentAdmissionsPolicy)} aria-label="Admissions" - disabled={isPending} + disabled={isPending && look !== "sheet"} > {admissionsPolicyOptions.map((p) => ( diff --git a/nextjs-app/components/FilterSheet.module.css b/nextjs-app/components/FilterSheet.module.css index 73491be..95c24db 100644 --- a/nextjs-app/components/FilterSheet.module.css +++ b/nextjs-app/components/FilterSheet.module.css @@ -74,10 +74,6 @@ box-shadow: inset 0 0 0 2px var(--brand); } -.segment input:disabled { - cursor: progress; -} - .footer { display: flex; align-items: center; diff --git a/nextjs-app/components/FilterSheet.tsx b/nextjs-app/components/FilterSheet.tsx index 6229a09..df72421 100644 --- a/nextjs-app/components/FilterSheet.tsx +++ b/nextjs-app/components/FilterSheet.tsx @@ -61,7 +61,6 @@ export function FilterSheet({ type="button" className={`btn btn-tertiary ${styles.clearAll}`} onClick={onClearAll} - disabled={isPending} > Clear all @@ -76,7 +75,9 @@ export function FilterSheet({ } > -
+ {/* Controls stay enabled while a change lands, so focus is never + dropped out of the dialog; aria-busy says the results are updating. */} +
{radius !== null && (
@@ -95,7 +96,6 @@ export function FilterSheet({ value={r} checked={r === radius} onChange={() => onRadiusChange(r)} - disabled={isPending} aria-label={`Within ${radiusLabel(r)}`} /> {r} mi