overhaul: 05 app shell - #19
Conversation
|
CS-5
left a comment
CS-5
left a comment
There was a problem hiding this comment.
Correctness review of the app shell. I built the branch at 1b55703 (linking sharp in from the store so the image step completed) and read the emitted HTML/CSS rather than the source alone, which is how the first finding turned up.
9 findings, most of them in Navbar.astro's mobile sheet.
The one worth acting on before merge: Icon.astro has no rest spread, so data-menu-icon-open / data-menu-icon-close never reach the DOM — the menu button's hamburger→X swap silently never happens. It doesn't fail typecheck because TypeScript doesn't check hyphenated JSX attribute names, and it doesn't fail at runtime because of the ?.. Verified in dist/index.html. This is a good argument for the primitives that get driven by script to spread ...rest.
Two more sheet bugs in the same area: the backdrop click handler is unreachable (<nav class="h-full"> fills the fixed inset-0 sheet, so event.target === sheet is never true), and the body scroll lock leaks if the viewport crosses md while the sheet is open — the sheet and its toggle both vanish via md:hidden and leave the page unscrollable.
The rest are smaller: no inert on the page behind the open sheet, isCurrent vs inSection disagreeing between the two navs, the stale-link guard silently skipping entries it can't parse, min-h-dvh without a flex column, the maskable manifest icon reusing the unpadded mark (measured: 5.9% padding, needs ~20%), and the og:image dimensions block never firing for the default card.
Nothing here is architectural — the layout/SEO shape is good, and putting description in the type signature is the right call. Verified separately and found clean: the .lycheeignore list is exactly the 10 root-relative links in dist that don't resolve (no more, no less), font preload URLs match the @font-face url()s hash-for-hash, canonical/noindex are correct on all three pages, the OG image is a proper 1200×630, and astro check is at 0 errors.
Generated by Claude Code
b3f088b to
485d52f
Compare
485d52f to
9e9e5ab
Compare
The chrome every page shares, with SEO correctness built into the type signature rather than left to each page: BaseLayout requires title and description, so omitting either is a compile error instead of something a reviewer has to catch. Seo emits title, description, canonical, the full Open Graph set with image dimensions, and a Twitter card. No keywords meta — legacy carried 22 terms for a signal search engines dropped years ago. jsonld.ts holds typed builders: NGO (the specific type for a nonprofit, not bare Organization) on every page, WebSite on the homepage, and breadcrumbs ready for the nested pages in Phases 07-08. Navbar follows D26: the real lockup, full-width on desktop and the square mark on mobile, never a rebuilt gear plus the name in Inter. Programs is a real link to /programs with a dropdown in addition, so touch and no-JS both get a destination — verified that the panel opens on keyboard focus and Tab moves into it. The mobile sheet is full-height with 44px targets, locks body scroll, and closes on Esc with focus returned. The only script is the ~25-line toggle. Footer is a recessed card band carrying the lockup and the engineering title block. It points Donate at the real /donate page rather than legacy's /wiki/donations, and adds a Programs column — with the mobile sheet closed, the footer is otherwise the only place a program link appears. sharp becomes a direct dependency: the first real <Image> use made astro:assets need it, which allowBuilds already anticipated. The icon set is rendered from the square mark rather than committed by hand; the apple-touch icon is flattened onto the brand ground because iOS ignores transparency. exactOptionalPropertyTypes forced one signature choice worth knowing: a layout that forwards optional props passes explicit undefined, which that flag treats as distinct from an absent prop, so the receiving props are declared `?: T | undefined` instead of filtering props at every call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YRfxMh7FLjQtDbb1BEsCbR
The app shell links to the full site's routes, but Phase 07 builds them, so the offline link check failed on every page carrying the header and footer: 65 errors across ten routes. That is a correct finding about an unavoidable ordering problem, not a tooling fault. .lycheeignore excludes exactly those ten routes and nothing else, so the gate stays live for assets, anchors, and any link outside the list — verified by seeding a bogus href, which still fails the check. An exclusion list like this is a trap once its pages land: the entry silently keeps that page out of the link check forever. tools/checks/stale-link-ignores.mjs fails CI if an excluded route now exists in dist, so the list can only shrink. Phase 07's acceptance criteria now require the file to be gone. Verified locally with lychee 0.24.2 using the workflow's exact flags: 0 errors, and the guard correctly fails when a landed route is left excluded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YRfxMh7FLjQtDbb1BEsCbR
Four things in the mobile menu did not work. `Icon.astro` has no rest spread, so `data-menu-icon-open`/`-close` never reached the DOM and the hamburger never became an X — the `?.` swallowed the nulls and TypeScript does not check hyphenated JSX attribute names. Backdrop close was unreachable: the sheet's `<nav class="h-full">` is exactly the sheet's box, so `event.target === sheet` could never be true. Crossing `md` with the sheet open left the page scroll-locked and unscrollable with no visible control, since both the sheet and its toggle are `md:hidden`. And nothing made the page behind the opaque sheet inert, so Tab past the last item walked every invisible link in `main` and `footer`. The sheet now keeps one piece of state — `aria-expanded` — with CSS deriving the icon swap and the scroll lock from it, a `matchMedia` listener closing it at `md`, and `inert` on everything in the shell that is not the toggle. Verified in a real browser: icon swap, Esc, scroll lock, inert (only the close button stays focusable), and the breakpoint close all behave. The Programs panel was a `:focus-within` disclosure with no dismissal path, which could not meet this phase's own "Esc closes" criterion (§9) or WCAG 2.2 SC 1.4.13 — and that criterion had been rewritten to describe what shipped. It is a native `popover` now: Esc, light-dismiss, top-layer and the `expanded` state come from the platform with no script, and the criterion is restored. The sticky header animated `height` on a scroll timeline, relayouting itself and reflowing the page on every frame through the first 120px, against §6's "only `opacity` and `transform` animate" — it animates `scale`. The header logo was `loading="lazy"`, invisible to the preload scanner despite being an LCP candidate; it is `priority`. `min-h-dvh` on a block `<body>` stretched nothing, leaving a lighter strip below the footer band on tall viewports; the body is a flex column and `main` absorbs the slack (measured: 0px gap at 1600px tall). `stale-link-ignores.mjs` silently dropped any entry it could not parse, so a malformed exclusion escaped the staleness guard forever while lychee still applied it — exactly the permanent blind spot it exists to prevent. It fails loud now. The `og:image:width`/`height` block was dead for every page, since the default `ogImage` is a string; the default card's dimensions travel with its path in `site.ts`. The manifest's maskable entry reused the unpadded 512, whose artwork spans ~88% of its box, so a launcher mask clipped the gear teeth — a padded render at 72% is a separate file. Quality: the route inventory was declared four times across two components and the 404 page, already diverged, with the Footer re-typing program names `site.ts` already held; it is one `nav` structure there, and the desktop/sheet split is a `surfaces` field rather than a discrepancy. DESIGN.md §8's link contract had no implementation home — twelve call sites wrote `no-underline` and no chrome link underlined on hover; `ui-link` and `external-link` are utilities, as is §5's container rule, which was a class string at five sites. The Footer declared its socials twice and re-implemented `CardTitle`. `site.webmanifest` is a generated route reading `site.ts` instead of restating the org name, a third description and both brand colours. `organization`/`webSite` are constants, not zero-argument factories. Dropping the `cn.ts`, `fonts.ts` and `site.ts` knip entries — all genuinely referenced as of this PR — restored export-level checking in the two most central modules; `jsonld.ts`'s entry became `@public` on the one unused export. Also fixed a regression this PR nearly introduced: the first draft toggled the menu icons from a scoped `<style>`, which Astro rewrites to demand this component's scope attribute — the same failure the Carousel had, since `Icon.astro` renders its own root. `:global()` on the icon side, verified in the built CSS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BX5PrKuYNRLVxiEj3eejhs
9e9e5ab to
1e0d0d8
Compare
2 participants
Layer 5 of the overhaul stack, on
overhaul/04-content-model.plan/05-app-shell.md.The chrome every page shares. This is the first layer where the site looks like the site — the preview URL now shows real header, footer, and metadata.
The part that matters most
SEO correctness is in the type signature, not in a review checklist.
BaseLayoutrequirestitleanddescription, so a page that omits one does not compile:That's the mechanism that stops Phase 07 from shipping the eight pages legacy shipped with missing metadata.
What's here
Seo.astro— title (templated, bare site name on the homepage), description, canonical, the full Open Graph set with image dimensions and alt, Twittersummary_large_image,theme-color. Nokeywordsmeta: legacy carried 22 terms for a signal that stopped mattering years ago.src/lib/jsonld.ts— typed builders, serialized through a tinyJsonLd.astro.NGO(the specific type for a nonprofit, not bareOrganization) site-wide,WebSiteon the homepage,breadcrumbs()ready for Phases 07–08.Navbar.astroper D26 — the real lockup, full-width on desktop and the square mark on mobile, never a rebuilt gear plus the name in Inter. Sticky, condensing after scroll via a scroll-driven animation behind@supportsandprefers-reduced-motion. Programs is a real link to/programswith a dropdown in addition, so touch and no-JS get a destination.Footer.astro— recessedcardband with the lockup, three link columns, socials, and the engineering title block at the bottom where a drawing sheet puts its identity strip.404.astro— branded and actually useful (the four places people were most likely headed), noindexed.Icons + manifest —
favicon.icocarried over,icon.svgfrom the square mark, 180/192/512 PNGs rendered from it with sharp, and a web manifest./and/styleguidenow render through the shell.Verified in a browser, not by eye
opacity: 1, next stop is "FLL")aria-expandedflips; sheet shows; body scroll locksheader, 1main, 1footer, 3 labelednavsNGOsite-wide +WebSiteon homepnpm check && pnpm buildgreen.One acceptance item I could not complete
The brief asks to paste the JSON-LD into Google's Rich Results test once.
search.google.comandvalidator.schema.orgare both blocked from this environment, so that manual check is outstanding. Phase 10 already tasks validating every distinct JSON-LD shape, so it lands there — flagging it rather than quietly ticking the box.Deviations
sharpis now a direct dependency. The first real<Image>use made astro:assets require it;pnpm-workspace.yaml'sallowBuilds: sharpalready anticipated exactly this.exactOptionalPropertyTypesforced a signature choice. A layout that forwards optional props passes an explicitundefined, which that flag treats as distinct from an absent prop. Receiving props are declared?: T | undefinedrather than filtering props at every call site — worth knowing before writing the next layout.opengraph-image.png, moved topublic/og/default.png. Phase 10 builds the curated per-section set; inventing a template now is work Phase 10 would redo./wiki/donations; this points at the real/donatepage and adds a Programs column (FLL, FRC, Calendar) — with the mobile sheet closed, the footer is otherwise the only place a program link appears. The 501(c)(3) line and everything else is verbatim.#262626because iOS ignores transparency and would composite it on black.Generated by Claude Code