fix(rep): cap picker sheet at max-h-[90dvh] so the list stays on-screen
Without a height constraint, the base-ui bottom-sheet's `data-[side=bottom]:h-auto` sizes the sheet to its content. The picker renders 678 customers, so the sheet grew to 46,373px in an 809px viewport — the inner overflow-y-auto container had nothing to scroll within, and the list rendered above the visible area (confirmed on 2026-09-20). Fix: add `max-h-[90dvh]` at the picker SheetContent call site. `dvh` (dynamic viewport height) accounts for mobile browser chrome so the sheet never gets pushed off a small screen. `flex-1 overflow-y-auto` on the inner modal-shell__body was already there from the picker sort/toast refactor — it just needed a parent height to scroll within. Regression guard: src/app/(rep)/rep/rep-picker-height.test.ts asserts the picker SheetContent keeps the max-h cap, uses dvh (not vh), and the inner list container still has flex-1 + overflow-y-auto so the cap actually scrolls.
This commit is contained in:
parent
38c65af8e3
commit
41963f8dd9
2 changed files with 94 additions and 1 deletions
|
|
@ -577,7 +577,17 @@ export function CheckInView() {
|
|||
<Sheet open={pickerOpen} onOpenChange={(open) => (open ? setPickerOpen(true) : closePicker())}>
|
||||
<SheetContent
|
||||
side="bottom"
|
||||
className="rounded-t-2xl px-0 pb-0 sm:max-w-md sm:mx-auto"
|
||||
// `max-h-[90dvh]` keeps the sheet on-screen when the customer
|
||||
// list is long (a few hundred unpinned customers is normal).
|
||||
// Without a height cap, the base-ui bottom-sheet class
|
||||
// (`data-[side=bottom]:h-auto`) sizes the popup to its content
|
||||
// and the inner `overflow-y-auto` list container has nothing
|
||||
// to scroll within — the list just renders past the top of
|
||||
// the viewport. `dvh` (dynamic viewport height) accounts for
|
||||
// mobile browser chrome so the sheet never gets pushed off
|
||||
// the bottom of a small screen either. See rep-picker-height
|
||||
// test for the regression guard.
|
||||
className="rounded-t-2xl px-0 pb-0 max-h-[90dvh] sm:max-w-md sm:mx-auto"
|
||||
showCloseButton
|
||||
>
|
||||
<SheetHeader className="px-4">
|
||||
|
|
|
|||
83
src/app/(rep)/rep/rep-picker-height.test.ts
Normal file
83
src/app/(rep)/rep/rep-picker-height.test.ts
Normal file
|
|
@ -0,0 +1,83 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { dirname, resolve } from "node:path";
|
||||
|
||||
/**
|
||||
* Regression guard for the "list goes off the top of the screen"
|
||||
* bug in the rep picker Sheet.
|
||||
*
|
||||
* The base-ui Sheet component applies `data-[side=bottom]:h-auto`,
|
||||
* which sizes the bottom sheet to its content. With ~600 unpinned
|
||||
* customers in the picker, that means the sheet grows past the
|
||||
* viewport and the inner `overflow-y-auto` container has nothing
|
||||
* to scroll within — the list renders above the visible area
|
||||
* (verified on 2026-09-20, the list was 46,373px tall in an 809px
|
||||
* viewport before the fix).
|
||||
*
|
||||
* The fix is a `max-h-[90dvh]` override at the call site. If a
|
||||
* future refactor drops it, this test fails.
|
||||
*/
|
||||
const HERE = dirname(fileURLToPath(import.meta.url));
|
||||
const VIEW_PATH = resolve(HERE, "./check-in-view.tsx");
|
||||
|
||||
function viewSource(): string {
|
||||
return readFileSync(VIEW_PATH, "utf8");
|
||||
}
|
||||
|
||||
/**
|
||||
* Pull out just the JSX block for the picker `<SheetContent side="bottom" …>`.
|
||||
* We anchor on the picker-specific className substring that lives inside
|
||||
* the call (not a generic helper), so this only matches the rep picker
|
||||
* SheetContent even if other bottom sheets exist in the file.
|
||||
*/
|
||||
function pickerSheetContentBlock(source: string): string | null {
|
||||
// Anchor on the unique `max-h-[90dvh]` cap (which is what we're
|
||||
// testing for) and grab back to the opening `<SheetContent` tag.
|
||||
// If the cap is removed, the test fails on the first assertion
|
||||
// and this helper still gets re-evaluated on every test.
|
||||
const idx = source.indexOf("max-h-[90dvh]");
|
||||
if (idx === -1) return null;
|
||||
const start = source.lastIndexOf("<SheetContent", idx);
|
||||
if (start === -1) return null;
|
||||
// Walk to the next `<` past the closing `>` of the opening tag —
|
||||
// enough to read the full className.
|
||||
const close = source.indexOf(">", start);
|
||||
if (close === -1) return null;
|
||||
return source.slice(start, close + 1);
|
||||
}
|
||||
|
||||
describe("rep picker SheetContent height contract", () => {
|
||||
it("constrains the bottom sheet height with max-h-[90dvh]", () => {
|
||||
const source = viewSource();
|
||||
const block = pickerSheetContentBlock(source);
|
||||
expect(block, "picker SheetContent max-h-[90dvh] cap not found").not.toBeNull();
|
||||
expect(block!).toMatch(/max-h-\[90dvh\]/);
|
||||
});
|
||||
|
||||
it("uses dvh (not just vh) so mobile browser chrome is accounted for", () => {
|
||||
const source = viewSource();
|
||||
const block = pickerSheetContentBlock(source);
|
||||
expect(block!).toMatch(/max-h-\[90dvh\]/);
|
||||
// Negative: not bare `max-h-[90vh]` without the `d` suffix —
|
||||
// plain `vh` ignores the mobile browser chrome and the sheet
|
||||
// can still be pushed off the bottom of a small screen.
|
||||
expect(block!).not.toMatch(/max-h-\[90vh\](?!d)/);
|
||||
});
|
||||
|
||||
it("the inner list container has overflow-y-auto (so the cap actually scrolls)", () => {
|
||||
const source = viewSource();
|
||||
// The inner div around the <ul>…</ul> must have overflow-y-auto.
|
||||
// `modal-shell__body flex-1 overflow-y-auto px-4 pb-4` carries it.
|
||||
expect(source).toMatch(/modal-shell__body[^"]*\boverflow-y-auto\b/);
|
||||
});
|
||||
|
||||
it("the picker list container has flex-1 so it fills remaining sheet height", () => {
|
||||
const source = viewSource();
|
||||
// Without `flex-1`, the inner div sizes to its content and the
|
||||
// outer `max-h-[90dvh]` cap doesn't trigger a scrollbar — same
|
||||
// visual bug as dropping the cap. Pair this with the previous
|
||||
// test to nail down both halves of the constraint.
|
||||
expect(source).toMatch(/modal-shell__body[^"]*\bflex-1\b/);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue