Uh oh!
There was an error while loading. Please reload this page.
fix(a11y): brand-dark text on pink accent CTAs (#96) - #116
Conversation
Swap white-on-pink (2.72:1) for text-brand-dark (6.40:1) on the shared primary Button and remaining page CTA/hero/submit surfaces, matching the header fix from philaconvalley#95. Extend contrast e2e to cover a primary Button on /about. Closesphilaconvalley#96
@KhyFee is attempting to deploy a commit to the PhilaCon Valley Team on Vercel. A member of the Team first needs to authorize it. |
KhyFee
commented
Aug 4, 2026
Friendly ping — Vercel reported this PR needs team authorization for the preview deploy. Happy to tweak the contrast swaps if review feedback comes back before that auth lands. Thanks for considering the a11y fix for #96! |
traksaw
left a comment
There was a problem hiding this comment.
Overview
Thanks for picking this up. The core change is right: #1A1A1A on #FF66A8 is 6.40:1 (vs. 2.72:1 for white), and the hover state accent-500#F07AAC is 6.70:1 — both clear AA for normal text, and it matches the header approach from #95.
Two things need attention before merge: the new test doesn't assert on what it claims, and six of the ten files touch sections that aren't pink.
Blocking
1. The new e2e test asserts on the wrong element
e2e/contrast.spec.ts:121 — page.locator('section.bg-accent-400 a').first() on /about.
/about has exactly one bg-accent-400 section (about.astro:293), and its first anchor is <Button href="/join" variant="secondary"> (about.astro:300) — bg-primary-300 text-brand-dark, which already passed before this PR. /about contains no variant="primary" Button at all, so the regression guard this PR is built around isn't guarding anything. It passes today, and it would still pass if Button primary reverted to text-white.
Suggested fix — target a real primary Button by test id rather than DOM order. Button.astro already accepts data-testid:
<!-- src/pages/join.astro:122 -->
<Buttonhref="/projects"variant="primary"data-testid="primary-cta"class="w-full">awaitpage.goto('/join');constcta=page.getByTestId('primary-cta');effectiveColours reads the element's own background, so the button doesn't need to sit inside a pink section for the assertion to be meaningful.
2. Six files restyle coral/purple sections, not pink ones
The description says "same swap on pink CTA/hero sections," but these heroes are not pink:
| File | Section | Actual background |
|---|---|---|
about.astro:16 | hero | bg-brand-coral#EF657F |
about.astro:243 | Who We Serve | bg-brand-coral |
events.astro:22 | hero | bg-brand-purple#B383C3 |
resources/index.astro:25 | hero | bg-brand-coral |
projects/index.astro:63 | hero | bg-brand-purple |
support.astro:16 | hero | bg-brand-purple |
In each case only the <p> changed; the wrapping <section class="... text-white"> and the <h1>/<h2> are untouched. The result is a white heading directly above dark-grey body copy on the same coral/purple panel — a visible regression on six pages. resources/index.astro:30 also still has a text-white/50 line in that same hero, now sitting between white and dark text.
Only join.astro:12 is genuinely bg-accent-400, and that one is handled correctly (section and subtitle both flipped).
Recommendation: revert the six non-pink paragraph changes and keep this PR scoped to accent-400, matching the title and #96. Purple/coral contrast is a real problem worth its own issue — white on #B383C3 is 3.02:1, which only barely clears the 3:1 large-text threshold, and the text-white/85 subtitles on purple land around 2.6:1 and genuinely fail. That deserves a deliberate fix rather than a partial one.
Non-blocking
- Redundant outline overrides.
outlinealready resolves toborder-2 border-current text-brand-dark. The newclass="!text-brand-dark !border-brand-dark hover:!bg-brand-dark/10"on the seven CTA outline buttons re-states the default; the only real delta is hover/10vs. the variant's/5. Dropping theclassprop entirely would remove seven!importantescapes and let the component own its styling. contact.astro:251duplicates the primary variant by hand. This is exactly why the fix needed touching in two places. Worth a follow-up to render<Button type="submit">(needs atypeprop on the component) so the token lives in one file.- Pink-on-pink. In these CTA sections a
variant="primary"button would beaccent-400on anaccent-400background — near-invisible except for the shadow. None of the changed CTAs currently useprimarythere (they usesecondary), so it isn't a bug today, but it's a trap for the next contributor.
Risks
Low blast radius — pure Tailwind class changes, no logic, no dependencies, no security surface. The realistic risk is visual: item 2 ships mixed-contrast headers/body on six pages, and item 1 means CI won't catch a future revert of the actual fix.
Verdict: the Button.astro / contact.astro / pink-CTA changes are good and should merge as-is. Please pull the six coral/purple paragraph edits back out, and re-point the e2e test at a real primary Button.
KhyFee
commented
Aug 4, 2026
Thanks for the thorough review, @traksaw — really helpful, and you're right on both blocking points. I'll:
Agree purple/coral contrast deserves its own deliberate pass — happy to open a separate issue once this lands. On non-blocking: I'll drop the redundant outline Pushing a fix commit shortly. |
KhyFee
commented
Aug 4, 2026
Pushed in c77078b:
Left the non-blocking outline |
Summary
Buttonprimary:text-white→text-brand-darkonbg-accent-400_bloge2e/contrast.spec.tsto assert a primary Button on/aboutMatches the approach already used in the header/event bar (#95).
Closes#96
Test plan
npm run buildnpx playwright test e2e/contrast.spec.ts(or CI equivalent)