From 5c6172754aa1a1e77ac3ce0f962dbdc659ccbe31 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 17 Aug 2026 21:43:14 +0200 Subject: [PATCH] refactor(header): move --hdr-h measurement into PageShell (S7 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the S7 mobile-fix code-review findings: - (altitude) The header-height→content-pad contract lived across two files with an implicit DOM coupling: Header measured itself and wrote --hdr-h onto `el.parentElement`, assuming PageShell rendered it as a direct child of the padded column. Move the whole contract into PageShell, which owns BOTH the header slot and the padded content wrapper: Header is now forwardRef and PageShell attaches a ref (via cloneElement), measures the header with a ResizeObserver, and writes --hdr-h straight onto the content wrapper that consumes it — no parent assumption, producer and consumer together. - (simplification) The `+ 8` gap is now documented as the header's own gap-y-2 row gap reused below it, not a bare magic number. - (test-coverage) PageShell.dom.test.tsx: stub ResizeObserver + the header's offset metrics and assert --hdr-h is published onto the content wrapper (and absent with no header). Re-verified live at 440 (mobile, no overlap) and desktop (one row). FE gate: tsc / knip / lint clean, 220 vitest pass. --- frontend/src/components/Header.tsx | 50 ++++++----------- .../src/components/PageShell.dom.test.tsx | 53 +++++++++++++++++++ frontend/src/components/PageShell.tsx | 46 ++++++++++++++-- 3 files changed, 113 insertions(+), 36 deletions(-) create mode 100644 frontend/src/components/PageShell.dom.test.tsx diff --git a/frontend/src/components/Header.tsx b/frontend/src/components/Header.tsx index f607e265..fd8e822b 100644 --- a/frontend/src/components/Header.tsx +++ b/frontend/src/components/Header.tsx @@ -1,4 +1,4 @@ -import { useLayoutEffect, useRef, type ReactNode } from "react"; +import { forwardRef, type ReactNode } from "react"; import { useTranslation } from "react-i18next"; import { Youtube } from "lucide-react"; import type { Me } from "../lib/api"; @@ -16,15 +16,18 @@ import { useChannels, useChannelsActions } from "./ChannelsProvider"; // The floating top header: a row of glass pills over the content — a ModuleName pill (label + // accent dot + back/forward arrows), the module's SearchBar where it has one, and the per-user // sync chip at the right end. -export default function Header({ - me, - onYtSearch, -}: { - me: Me; - // Trigger a live YouTube search for the current term (feed only; hidden for the demo account, - // which can't spend the shared quota). - onYtSearch: (q: string) => void; -}) { +// `forwardRef` so PageShell can attach a ref to the header's root box and measure its real (possibly +// wrapped) height — that measurement + the `--hdr-h` content pad it drives live in PageShell, which +// owns both the header slot and the padded content wrapper (E5 S7). +const Header = forwardRef< + HTMLDivElement, + { + me: Me; + // Trigger a live YouTube search for the current term (feed only; hidden for the demo account, + // which can't spend the shared quota). + onYtSearch: (q: string) => void; + } +>(function Header({ me, onYtSearch }, ref) { const { t } = useTranslation(); const { page } = useNavigation(); const { setPage } = useNavigationActions(); @@ -46,27 +49,6 @@ export default function Header({ // Drop input focus on Go/Enter for the live-filter modules (feed's Go escalates to YouTube). const blur = () => (document.activeElement as HTMLElement | null)?.blur?.(); - // The header floats (absolute) over the content, which pads its top by `--hdr-h`. That default is - // sized for a single row; on a narrow window (esp. mobile) the flex-wrap header grows to 2–3 rows, - // so a FIXED pad would let the content toolbar (sort / count / filter chips) slide up under it. - // Measure the real header box and publish its height as `--hdr-h` on the content column, so the pad - // always tracks the actual (wrapped) header height at any width. - const headerRef = useRef(null); - useLayoutEffect(() => { - const el = headerRef.current; - const host = el?.parentElement; // the `relative` content column (PageShell renders a fragment) - if (!el || !host) return; - const apply = () => - host.style.setProperty("--hdr-h", `${el.offsetTop + el.offsetHeight + 8}px`); - apply(); - const ro = new ResizeObserver(apply); - ro.observe(el); - return () => { - ro.disconnect(); - host.style.removeProperty("--hdr-h"); - }; - }, []); - // Per-page SearchBar wiring — null for modules without a search. let search: ReactNode = null; if (page === "feed") { @@ -116,7 +98,7 @@ export default function Header({ // actual left zone: it sits just inside the content, reclaiming the space when the nav rail is // slim and/or no filter sidebar is present — no hard-coded offset.
); -} +}); + +export default Header; diff --git a/frontend/src/components/PageShell.dom.test.tsx b/frontend/src/components/PageShell.dom.test.tsx new file mode 100644 index 00000000..8b526bc4 --- /dev/null +++ b/frontend/src/components/PageShell.dom.test.tsx @@ -0,0 +1,53 @@ +// @vitest-environment jsdom +// +// E5 S7: PageShell measures the floating header and publishes its real height as `--hdr-h` onto the +// content wrapper, so a header that flex-wraps to 2–3 rows on a narrow window never overlaps the +// content below it. jsdom does no layout (offsetHeight is 0) and has no ResizeObserver, so we stub +// both: a ResizeObserver that hands us its callback, and offset metrics on the measured header. +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { render, cleanup } from "@testing-library/react"; +import PageShell from "./PageShell"; + +vi.mock("./PageScroller", () => ({ + default: ({ children }: { children: React.ReactNode }) =>
{children}
, +})); + +let roCb: (() => void) | null = null; +beforeEach(() => { + roCb = null; + vi.stubGlobal( + "ResizeObserver", + class { + constructor(cb: () => void) { + roCb = cb; + } + observe() {} + disconnect() {} + }, + ); +}); +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); + +describe("PageShell --hdr-h (E5 S7)", () => { + it("publishes the measured header height onto the content wrapper", () => { + const { container } = render( + H
}>content, + ); + const content = container.querySelector('[class*="pt-(--hdr-h)"]') as HTMLElement; + expect(content).toBeTruthy(); + // Give the header a real (multi-row) box, then fire the ResizeObserver callback the effect made. + const hdr = container.querySelector('[data-testid="hdr"]') as HTMLElement; + Object.defineProperty(hdr, "offsetTop", { value: 12, configurable: true }); + Object.defineProperty(hdr, "offsetHeight", { value: 96, configurable: true }); + roCb?.(); + expect(content.style.getPropertyValue("--hdr-h")).toBe("116px"); // 12 + 96 + 8 + }); + + it("adds no `--hdr-h` pad when there is no header", () => { + const { container } = render(content); + expect(container.querySelector('[class*="pt-(--hdr-h)"]')).toBeNull(); + }); +}); diff --git a/frontend/src/components/PageShell.tsx b/frontend/src/components/PageShell.tsx index a530eccd..98d302a7 100644 --- a/frontend/src/components/PageShell.tsx +++ b/frontend/src/components/PageShell.tsx @@ -1,4 +1,15 @@ -import { createContext, useContext, useState, type ReactNode } from "react"; +import { + cloneElement, + createContext, + isValidElement, + useContext, + useLayoutEffect, + useRef, + useState, + type ReactElement, + type ReactNode, + type Ref, +} from "react"; import { createPortal } from "react-dom"; import PageScroller from "./PageScroller"; @@ -52,10 +63,39 @@ export default function PageShell({ fadeTop?: boolean; }) { const [toolbarSlot, setToolbarSlot] = useState(null); + // The floating header pads the content below it by `--hdr-h`. That must equal the header's REAL + // height, which grows as the flex-wrap header wraps to 2–3 rows on a narrow window — a fixed pad + // would let the content toolbar slide up under it (E5 S7). Measure the header (a ref forwarded onto + // its root box) and write its height straight onto the content wrapper that consumes `--hdr-h`, so + // the two live together with no assumption about the header's DOM parent. + const headerRef = useRef(null); + const contentRef = useRef(null); + useLayoutEffect(() => { + const h = headerRef.current; + const c = contentRef.current; + if (!h || !c) return; + // `+ 8` = the header's own gap-y-2 row gap (0.5rem), reused as the gap below it. + const apply = () => c.style.setProperty("--hdr-h", `${h.offsetTop + h.offsetHeight + 8}px`); + apply(); + const ro = new ResizeObserver(apply); + ro.observe(h); + return () => { + ro.disconnect(); + c.style.removeProperty("--hdr-h"); + }; + }, []); + const measuredHeader = isValidElement(header) + ? cloneElement(header as ReactElement<{ ref?: Ref }>, { + ref: headerRef, + }) + : header; return ( - {header} -
+ {measuredHeader} +
{banners}