fix(web): reorder header, pad mobile sheet, repoint dead GitBook links - #154
Draft
heyoub wants to merge 9 commits into
Draft
fix(web): reorder header, pad mobile sheet, repoint dead GitBook links#154heyoub wants to merge 9 commits into
heyoub wants to merge 9 commits into
Conversation
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>
This was referenced Aug 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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