fix(web): reorder header, pad mobile sheet, repoint dead GitBook links - #154
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Issue CodeForPhilly#153 walks the live site on desktop and mobile and collects five findings. Four are shippable together because they all land in the app shell or in one screen's outbound links; the fifth (replace the Home hero CTA with a mailing-list invite) has no mechanism to build against, so the plan records the block up front rather than inventing one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Specs lead, so both files change before the code does. app-shell: the header's single "primary nav" table conflated content navigation with utilities, which is why the Volunteer CTA ended up buried between Members and About. Splitting it into a content cluster and a right-pinned utility cluster makes the CTA's position a stated rule rather than an accident, and gives the GitHub link and the auth control a declared home. Also states the sheet's accessible name and the icon-only-controls labelling rule, both of which the header violated. volunteer: the whole codeforphilly.gitbook.io space now returns 404 "Content owner not found", so the spec was prescribing two dead targets. The Meetup group and the CodeForPhilly/partnerships first-steps doc are the live equivalents; both verified reachable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Site check (CodeForPhilly#153) found the Volunteer button buried mid-nav between Members and About, where it read as one more section link rather than the call to action. It now closes the utility cluster, after the auth control, per the reordered app-shell spec. About joins the content links; a GitHub icon link is added to both breakpoints. The mobile sheet had no horizontal padding at all: SheetContent's base classes carry none and the only override here was a pt-8 hack, so nav items and the search box sat flush against the panel edge. Replaced with the structure shadcn intends — SheetHeader + SheetTitle, which bring their own p-4 — plus explicit px-4 on the nav and search. SheetTitle also gives the underlying Radix dialog the accessible name it never had. Three ARIA defects fixed while the file was open: aria-label on a roleless skeleton div (prohibited; now aria-hidden), a hand-written aria-expanded duplicating what Dialog.Trigger already supplies, and an aria-label overriding the About trigger's own visible text. The account-menu label stays — below sm the person's name is display:none, so it is the only accessible name there. Per-child ml-1 margins are gone; the parent gap-2 is now the single source of spacing at the same effective density. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The whole codeforphilly.gitbook.io space now returns 404 "Content owner not found", so both of the Volunteer screen's outbound CTAs were dead. PR CodeForPhilly#128 fixed the Home screen's copy of the same URL; these are the last two in the SPA. "When we meet" goes to the Meetup group, which is where hack nights are actually announced and which the footer already links to; "Read the guide" goes to the partnerships repo's first-steps doc, the surviving source of the GitBook page it replaces. The footer's "view this site on GitHub" link still named the repo codeforphilly-rewrite. That only resolves through GitHub's rename redirect, which is not something to depend on indefinitely. New Volunteer test asserts both hrefs and, following the Home dead-link idiom, that no gitbook.io URL survives anywhere in the rendered screen — so a copy-paste of the old constant cannot come back unnoticed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The sheet is a full-height flex column with no scroll container, so a nav list taller than the viewport was simply unreachable below the fold. That was already latent; adding the GitHub row makes it one row likelier on short phones. min-h-0 lets the flex child shrink at all, and overflow-y-auto gives it somewhere to put the excess. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ticks the criteria verified during implementation and, deliberately, leaves two unticked rather than rewriting them to match what was achievable: the browser pass belongs to whoever does UI QA, and the all-workspaces test gate cannot close on this Windows dev box. Notes record why. apps/api fails ten tests here on a tree whose API code is byte-identical to develop, because the fixtures assume POSIX — store.test.ts injects a write failure via /dev/null/impossible-path, which Windows will happily create, so the expected rejection never comes. Reproduces with the files run alone, so it is not runner contention. Filed as a follow-up rather than fixed: cross-platform fixtures are their own scope, and silently ticking a gate that did not run is worse than leaving the box open. Also flags HomeStub.tsx, which carries the same stale repo URL the footer had but is imported by nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins the numbers behind the unticked all-workspaces gate so a future reader can tell a known Windows baseline from a real regression, and notes the develop re-run that confirms it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Desktop header verified in headed Chrome at 1400px; the sheet verified via its portal at desktop width because the automation harness could not shrink a maximized window below md — same limitation web-shell.md's plan recorded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The repo was renamed to codeforphilly-ng; the old github.com/CodeForPhilly/codeforphilly-rewrite URLs in the setup snippets and the cutover announcement are a hard 404. Only the GitHub URLs change here — the k8s namespace and package name still carry the old slug on purpose. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
Nothing has imported or routed it since the scaffold (every live screen lives in src/screens/), and it carried the last stale codeforphilly-rewrite link in the web bundle. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
Three synchronous tests returned while AuthProvider's /api/auth/me fetch was still in flight, so its state update landed outside act() and warned on every run. Each now awaits the settled DOM the way the fourth test already does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
AppHeader.tsx and LoginPlaceholder.tsx carried byte-identical copies of the same SVG. One component under components/icons/ keeps the path data in a single place for the next screen that needs it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
The eleven per-item onClick closers missed the inline SearchBox's Enter path, so "every item in the sheet closes the sheet when activated" (specs/behaviors/app-shell.md) did not hold there. Derive the open state from the location.key the sheet was opened at instead: any client-side navigation closes it, including a same-path re-click. Derived during render rather than in a useLocation effect because react-hooks/set-state-in-effect flags the effect form; this is the same state-sync pattern ProjectEdit.tsx uses. The two anchors that do not change location (Contact mailto:, GitHub new tab) keep an explicit closer. Tests cover a sheet link click and the search Enter path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
Radix warned on every open that the dialog's aria-describedby pointed at nothing; a visually-hidden SheetDescription satisfies it. The className="w-72" on SheetContent never applied — the primitive's data-[side=right]:w-3/4 and sm:max-w-sm win — so it goes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
Volunteer and the GitHub link left "Primary navigation" in this PR and landed in a plain div, so they fell out of every navigation landmark. A second nav labelled "Utility" keeps them reachable by landmark without moving them back; app-shell.md's Accessibility section now names both landmarks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
Tailwind's container is 768px wide across the whole 768–1023 range (736px content box). Measured at 768: logo 128 + nav 297 + gaps 48 + utility cluster 387 = 860, so the document scrolled to 845px and "Help Wanted" wrapped onto two lines. Hide the GitHub icon link below lg (the mobile sheet keeps its own GitHub row) and narrow the desktop search to w-24 there, restoring w-48 / focus-within:w-72 at lg. The results dropdown is anchored right-0 with min-w-72 instead of stretching left-0 right-0, so it stays 288px wide over the narrow input — the width it already had at lg. After: 728px used at 768, no horizontal scroll; 1024 fits with the icon and wide search back. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
Restore the browser-test criterion to its original wording and leave it unchecked — the sheet and the < md header bar were never exercised at a < md viewport, and the earlier rewrite leaned on a "fixed w-72" claim that was wrong (the sheet is w-3/4 capped at sm:max-w-sm). Notes now say exactly what was and was not verified, including the headless 768/1023/1024 tablet measurements from review, and point at CodeForPhilly#16. Replace the closeout placeholders with the review fixes (location-keyed sheet close, lg gating, Utility landmark, GitHubIcon lift, HomeStub removal, stale-URL sweep, Volunteer test awaits), drop the HomeStub follow-up now that the file is gone, and convert the unfiled cross-platform-fixtures follow-up into issue CodeForPhilly#162 plus a new CodeForPhilly#163 for the pre-existing signed-in overflow in the same tablet band. Test counts reflect the Linux run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
8c94dc8 to
684ca34
Compare
|
Thanks @heyoub, this is a careful PR. The spec-first split, the plan record, and the ARIA cleanups all landed well, and the Windows note in the plan was genuinely useful. I pushed a few fix-up commits on top of your branch (rebased onto the current Mobile sheet closes on any navigation. The eleven per-item Sheet dialog description. Radix warned on every open about a missing Tablet band (768–1023px) overflow. Tailwind's Utility landmark. The desktop utility cluster is now Smaller things. Plan closeout. Per the plans protocol, the browser-test criterion is restored to its original wording and left unchecked (what was and was not exercised is spelled out in Notes, and the Validation: 🤖 Generated with Claude Code |
Review follow-ups on the combobox rewrite: - Drop the Home/End branches. In an editable combobox those keys move the text caret (APG); hijacking them for the listbox surprised anyone editing the query. - activate() now blurs the input, so focus does not stay parked in an emptied combobox after choosing a result. The Enter fallback reuses seeAllUrl + activate() instead of repeating the same four steps. - Options track the pointer with a guarded onMouseMove instead of onMouseEnter: results arriving under a stationary pointer would otherwise steal the highlight from a keyboard user. - The inline (mobile-sheet) instance renders its results in-flow with a smaller cap rather than absolutely positioned, so on short phones the list can no longer hang below the sheet's fixed-height viewport. AppHeader's test queries the sheet search as a combobox now, and gains a case for clicking a result closing the sheet (via CodeForPhilly#154's location-key mechanism). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
Check the validation boxes actually verified (new combobox tests, the audited aria-label/aria-invalid sets, and the full gate: type-check, lint, api 434/434, web 114/114, shared 75/75), fill in the closeout Notes (mousedown-swallow vs focus-out decision, hardcoded error ids kept, the post-CodeForPhilly#154 review pass, and that the mobile-sheet layout was reasoned rather than screenshotted), and convert the prose Follow-ups into taxonomy shapes: CodeForPhilly#164 (useId-derived form error ids) and CodeForPhilly#165 (shared combobox hook) filed and linked; the Retry contradiction is resolved in this PR. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr
The accessibility audit's third bucket — findings with one obviously-correct fix each and no design decision attached. Recording it as a plan before touching code so the DAG carries the scope, the PR stack it sits on (CodeForPhilly#154 + CodeForPhilly#155), and its relationship to issue CodeForPhilly#156, which holds the design-decision findings that are deliberately NOT implemented here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Addresses #153 (three of its four items; the fourth is documented below rather than built).
Spec-first per
specs/README.md:specs/behaviors/app-shell.mdandspecs/screens/volunteer.mdchange in this PR before the code, andplans/site-check-153.mdis the plan record.What changed
Projects · Help Wanted · Members · About ▾) next to the logo, and a right-pinned utility cluster (GitHub icon · Search · Sign in · Volunteer), with the green Volunteer button rightmost per the issue's recommendation. The redundantml-1-on-top-of-gap-1spacing is gone (singlegap-2).https://github.com/CodeForPhilly("can't hurt either", per the issue). Also in the mobile sheet as a text row.SheetContentships no padding and the header only addedpt-8). Now uses the intended shadcn structure:SheetHeader+SheetTitle("Menu") +px-4on nav/search. TheSheetTitlealso gives the Radix dialog its missing accessible name. Every item now closes the sheet, including the Contactmailto:.codeforphilly.gitbook.iospace 404s ("Content owner not found").Volunteer.tsx's two constants now point at live equivalents: "When we meet →" → the Meetup group (same target as the footer's Meetup icon), and "Read the guide →" → the surviving canonical source,CodeForPhilly/partnerships/.../first-steps.md. Same fix PR fix(web): route anonymous "Start a Project" CTA to login #128 made on the Home screen; this was the leftover twin. NewVolunteer.test.tsxcarries the dead-link regression assertion.codeforphilly-rewriterepo URL →codeforphilly-ng.aria-hidden, About trigger named by its visible text, hand-writtenaria-expandedremoved (Radix supplies it).Not implemented, deliberately
The issue's "mailing list invite in hero" recommendation is blocked: there is no anonymous mailing-list mechanism anywhere (only the auth-gated
/accountcheckbox), and public newsletter signup is explicitly deferred by spec (app-shell.mdConnect column,deferred.md). Recorded in the plan's Follow-ups — it needs a spec + mechanism decision first, and the hero is untouched here.Judgment calls for review
Validation
type-check,lint, web tests 96/96, shared 75/75. Headed-Chrome check of the desktop header and the open sheet (screenshots in the plan's validation notes).apps/apitests were not affected (no api files changed); note thatstore.test.tshas a pre-existing Windows-only failure —/dev/null/impossible-pathis a creatable path on Windows — unrelated to this PR and only relevant to Windows contributors.🤖 Generated with Claude Code