Skip to content

Commit 684ca34

Browse files
chore(plans): record review closeout for site-check-153
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 #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 #162 plus a new #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
1 parent d5a4221 commit 684ca34

1 file changed

Lines changed: 63 additions & 27 deletions

File tree

‎plans/site-check-153.md‎

Lines changed: 63 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -119,17 +119,9 @@ One-line repo URL swap to `codeforphilly-ng`.
119119
- [x] Footer "view this site on GitHub" points at `codeforphilly-ng`.
120120
- [x] Both replacement URLs return 200 and carry the expected content.
121121
- [x] `npm run -w packages/shared build`, `npm run type-check`, and `npm run lint` clean.
122-
- [x] `npm test` clean for the workspaces this plan touches: web 96/96, shared 75/75.
123-
- [ ] `npm test` clean for **all** workspaces — `apps/api` cannot pass on the Windows dev box used here (see Notes); needs a Linux run or CI to close.
124-
- [x] Browser test (headed Chrome, Vite dev server): desktop header order,
125-
spacing, GitHub icon, and Volunteer-rightmost verified at 1400px; the
126-
sheet verified open — "Menu" title, padded nav/search, no title/close
127-
collision — and Escape closes it. Caveat: the harness could not shrink
128-
the (maximized) window below md, so the sheet was opened via its
129-
CSS-hidden trigger at desktop width. The sheet is a fixed `w-72`
130-
portal, so its rendering is identical at < md; the < md *header bar*
131-
(logo + auth + hamburger row) still rides on the jsdom tests, same
132-
limitation `web-shell.md:109` recorded.
122+
- [x] `npm test` clean for the workspaces this plan touches: web 98/98, shared 75/75.
123+
- [x] `npm test` clean for **all** workspaces — api 434/434 on Linux at review (the Windows dev box could not run it, see Notes). Two web files (`ProjectEdit`, `ExpressInterestModal`, neither touched here) hit 5s timeouts under full-suite load and pass when run alone.
124+
- [ ] Browser test: desktop header order + mobile sheet padding at < md, both breakpoints.
133125

134126
## Risks
135127

@@ -143,8 +135,56 @@ One-line repo URL swap to `codeforphilly-ng`.
143135

144136
## Notes
145137

146-
(To be populated at closeout. Recorded during implementation:)
147-
138+
- **Browser criterion left unchecked.** What was exercised: desktop header
139+
order, spacing, GitHub icon, and Volunteer-rightmost in headed Chrome at
140+
1400px; the sheet opened via its CSS-hidden trigger at desktop width ("Menu"
141+
title, padded nav/search, no title/close collision, Escape closes it); and,
142+
at review, headless Chrome at 768/1023/1024px for the tablet band (below).
143+
What was not: the sheet and the < md header bar at an actual < md viewport.
144+
The sheet is not a fixed-width portal — `ui/sheet.tsx` sizes it
145+
`w-3/4` of the viewport capped at `sm:max-w-sm` — so its desktop-width
146+
rendering is not evidence for < md. Same gap `web-shell.md:109` recorded;
147+
covered by issue [#16](https://github.com/CodeForPhilly/codeforphilly-ng/issues/16).
148+
- **Sheet closes on navigation, not per item.** Review found the eleven
149+
hand-wired `onClick={() => setMobileOpen(false)}` closers missed the inline
150+
SearchBox's Enter path. Replaced with one rule: `mobileOpen` is derived as
151+
"opened at the current `location.key`", so any client-side navigation
152+
(NavLink, search Enter, same-path re-click) closes it. Derived during render
153+
rather than a `useLocation` effect because `react-hooks/set-state-in-effect`
154+
(in the plugin's v7 recommended set) flags the effect form — same
155+
state-sync pattern `ProjectEdit.tsx` uses. Explicit closers remain only on
156+
the two anchors that don't change location (Contact `mailto:`, GitHub
157+
new-tab). Two tests cover a NavLink click and the search Enter path.
158+
- **Tablet band (768–1023px) overflowed.** Tailwind's `container` is 768px
159+
wide across that whole range (736px content box). Measured at 768: logo 128
160+
- nav 297 + gaps/margins 48 + utility cluster 387 = 860, so the document
161+
scrolled to 845px and "Help Wanted" wrapped. Fix: GitHub icon link is
162+
`hidden lg:inline-flex` (the sheet keeps its own row) and the desktop
163+
SearchBox is `w-24 lg:w-48 lg:focus-within:w-72`. Its results dropdown is
164+
now anchored `right-0 min-w-72` instead of `left-0 right-0`, so it stays
165+
288px wide over the narrow input — the same width it had at `lg` before.
166+
After: 728px used at 768 (no scroll, one-line nav); 1024 fits with the icon
167+
and 192px search restored. Signed-in accounts still overflow this band
168+
(pre-existing, see Follow-ups).
169+
- **Mobile sheet dialog.** Radix warned on every open about a missing
170+
description; a visually-hidden `SheetDescription` ("Site navigation") now
171+
satisfies `aria-describedby`. The `className="w-72"` on `SheetContent` was
172+
dead (the primitive's `data-[side=right]:w-3/4` / `sm:max-w-sm` win) and is
173+
gone; the plan's earlier "fixed `w-72`" claim was wrong.
174+
- **Utility landmark.** The desktop utility cluster is `<nav aria-label="Utility">`
175+
so Volunteer and GitHub stay inside a navigation landmark after leaving
176+
"Primary navigation"; app-shell.md's Accessibility section names both.
177+
- **`GitHubIcon` lifted** to `apps/web/src/components/icons/GitHubIcon.tsx`;
178+
the byte-identical copies in `AppHeader.tsx` and `LoginPlaceholder.tsx` now
179+
import it. (Approach §2 above describes the copy as originally planned.)
180+
- **`HomeStub.tsx` removed** here after all — zero importers (grep-verified),
181+
dead since scaffold. The same stale `codeforphilly-rewrite` GitHub URL was
182+
also repointed in `specs/architecture.md`, `specs/behaviors/storage.md`, and
183+
`docs/operations/cutover-announcement.md` (a hard 404, unlike the k8s
184+
namespace / package name uses of the old slug, which are left alone).
185+
- **`Volunteer.test.tsx` act warnings.** Three synchronous tests returned
186+
before `AuthProvider`'s `/api/auth/me` fetch resolved; each now awaits the
187+
settled DOM like the fourth test.
148188
- **`apps/api` tests do not pass on Windows, independent of this plan.**
149189
`apps/api` finishes 3 failed | 30 passed (33 files), 10 failed | 413 passed
150190
(423 tests) — the ten spread across `scrub-data.test.ts` (4),
@@ -163,8 +203,6 @@ One-line repo URL swap to `codeforphilly-ng`.
163203

164204
## Follow-ups
165205

166-
(To be populated at closeout. Known now:)
167-
168206
- **Tracked as: blocked — hero "mailing list invite" CTA.** Issue #153
169207
recommends replacing the Home hero's Volunteer CTA with a mailing-list
170208
invite. There is no anonymous mailing-list mechanism anywhere in the repo:
@@ -178,15 +216,13 @@ One-line repo URL swap to `codeforphilly-ng`.
178216
spec would invent unspecified behavior. `Home.tsx` is deliberately untouched
179217
here; the CTA swap should follow the newsletter spec work, not precede it.
180218

181-
- **Tracked as: dead file, not fixed here — `apps/web/src/pages/HomeStub.tsx`.**
182-
It carries the same stale `codeforphilly-rewrite` URL the footer had, but
183-
nothing imports or routes it (`App.tsx` imports only `LoginPlaceholder` from
184-
`src/pages/`; every live screen lives in `src/screens/`). Left alone because
185-
the right fix is deleting the file, not patching a URL nobody renders — and
186-
that deletion wants its own scope. Flagging so a future grep for the old repo
187-
name doesn't read as an unfixed live link.
188-
189-
- **Issue — make the `apps/api` test fixtures cross-platform.** The `/dev/null`
190-
failure-injection idiom (and whatever the other seven failures share) makes
191-
the API suite unrunnable on a Windows dev box, so the documented validation
192-
gate can only be completed on Linux or in CI. See Notes for the mechanism.
219+
- Issue [#162](https://github.com/CodeForPhilly/codeforphilly-ng/issues/162) — make
220+
the `apps/api` test fixtures cross-platform. The `/dev/null` failure-injection
221+
idiom (and whatever the other seven failures share) makes the API suite
222+
unrunnable on a Windows dev box, so the documented validation gate can only be
223+
completed on Linux or in CI. See Notes for the mechanism.
224+
225+
- Issue [#163](https://github.com/CodeForPhilly/codeforphilly-ng/issues/163) — the
226+
signed-in header (avatar + full name + caret) still overflows the 768–1023px
227+
band; predates this plan and needs a breakpoint the shared `AuthControls`
228+
can apply to the desktop cluster only.

0 commit comments

Comments
 (0)