fix(search): keep the filter sheet usable while a change lands
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m13s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 32s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m19s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 1m14s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 20s
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m13s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 32s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m19s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 1m14s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 20s
The sheet's controls were disabled while a filter change navigated, and a control disabled under focus drops it to <body>, out of the dialog. They now stay enabled, with aria-busy on the sheet instead. The disabling had also been covering a race: updateURL built from useSearchParams, which only catches up once a navigation lands, so a second change made before then undid the first. It now builds on the URL the navigation in flight is heading to. The sheet also closes if the screen widens past phone width while it is open, so its selects and the desktop row's are never both showing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
cf3c773f86
commit
fa49164143
4 files changed
+118
-14
No files matched your search
@@ -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 <body>, 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(<FilterBar filters={filters} />);
|
||||
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(<FilterBar filters={filters} />);
|
||||
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(<FilterBar filters={filters} />);
|
||||
openSheet();
|
||||
act(() => listeners.forEach((l) => l({ matches: false })));
|
||||
expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
@@ -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<string | null>(null);
|
||||
useEffect(() => {
|
||||
if (!isPending) pendingQueryRef.current = null;
|
||||
}, [isPending]);
|
||||
|
||||
const updateURL = useCallback(
|
||||
(updates: Record<string, string>) => {
|
||||
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<FilterKey, string> = {
|
||||
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 <body>, 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"}
|
||||
>
|
||||
<option value="">Any phase</option>
|
||||
{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"}
|
||||
>
|
||||
<option value="">Any school type</option>
|
||||
{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"}
|
||||
>
|
||||
<option value="">All Local Authorities</option>
|
||||
{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"}
|
||||
>
|
||||
<option value="">Boys, Girls & Mixed</option>
|
||||
{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"}
|
||||
>
|
||||
<option value="">With or without sixth form</option>
|
||||
<option value="yes">With sixth form</option>
|
||||
@@ -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"}
|
||||
>
|
||||
<option value="">All admissions types</option>
|
||||
{admissionsPolicyOptions.map((p) => (
|
||||
|
||||
@@ -74,10 +74,6 @@
|
||||
box-shadow: inset 0 0 0 2px var(--brand);
|
||||
}
|
||||
|
||||
.segment input:disabled {
|
||||
cursor: progress;
|
||||
}
|
||||
|
||||
.footer {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
|
||||
@@ -61,7 +61,6 @@ export function FilterSheet({
|
||||
type="button"
|
||||
className={`btn btn-tertiary ${styles.clearAll}`}
|
||||
onClick={onClearAll}
|
||||
disabled={isPending}
|
||||
>
|
||||
Clear all
|
||||
</button>
|
||||
@@ -76,7 +75,9 @@ export function FilterSheet({
|
||||
</div>
|
||||
}
|
||||
>
|
||||
<div className={styles.body}>
|
||||
{/* Controls stay enabled while a change lands, so focus is never
|
||||
dropped out of the dialog; aria-busy says the results are updating. */}
|
||||
<div className={styles.body} aria-busy={isPending}>
|
||||
{radius !== null && (
|
||||
<div className={styles.field}>
|
||||
<span id={distanceId} className={styles.fieldLabel}>
|
||||
@@ -95,7 +96,6 @@ export function FilterSheet({
|
||||
value={r}
|
||||
checked={r === radius}
|
||||
onChange={() => onRadiusChange(r)}
|
||||
disabled={isPending}
|
||||
aria-label={`Within ${radiusLabel(r)}`}
|
||||
/>
|
||||
<span>{r} mi</span>
|
||||
|
||||
Reference in new issue
Block a user