Repository navigation
Conversation
philorien
left a comment
philorien
left a comment
There was a problem hiding this comment.
Thanks for this. The useId() approach is the right fix and is SSR-stable on Nuxt 4 / Vue 3.5. A few things need to change before merge, though: the fix is incomplete for #80, and one rewritten line introduces a new a11y problem. Inline comments below cover the files in this diff. Three issues are in files this PR doesn't touch, so they're listed here instead:
1. app/components/ContactSection.vue (~line 79): still has hardcoded IDs. ContactSection is rendered once per contactSectionBlock by PageSections, the same as the components fixed here, but it still uses id="contact-heading" and err-firstName, err-email, etc. With two contact blocks on one page, both sections are labelled by the first heading, and the second form's aria-describedby points at the first form's error spans, so screen readers announce the wrong errors. Please switch these to useId() as well (one base ID per instance, e.g. ${id}-err-email).
2. app/components/fridge/FridgeClosingCta.vue (~line 7): still has closing-heading. This is rendered per ctaSection by PageSections and also directly on full-hearts-fridge.vue. Two CTA blocks (or the fridge page plus a CMS CTA) produce duplicate id="closing-heading", which is exactly the bug #80 describes. Same useId() fix applies.
3. app/components/home/ActivityGrid.vue lines 23-25: invalid void-element closing tags. <br></br> and <hr></hr> aren't valid HTML. Browsers parse </br> as a second <br>, so each one produces two line breaks instead of one, and </hr> is a parse error. Please replace these with spacing CSS (e.g. margin on .items / .btn) or a single <hr>.
Also, app/components/page/PageSections.vue:42 passes section.heading || '' to HomeActivityGrid; see the inline comment on ActivityGrid.vue for that one.
|
@jay23606 thanks for the update. I re-reviewed 42ad06f: the There's one blocker left before I can approve. Please review and fix:
<section class="closing" aria-labelledby="v-1"><h2 id="v-1" class="display"></h2><p class="note"></p>…</section>This is the same empty-heading problem (an unnamed region) that you fixed for the card grid. Please use the same pattern here: defineProps<{ heading?: string; note?: string; cta: Cta; secondaryCta?: Cta }>()<section class="closing" :aria-labelledby="heading ? headingId : undefined">
<h2 v-if="heading" :id="headingId" class="display">{{ heading }}</h2>
<p v-if="note" class="note">{{ note }}</p>Then pass Dropping #3 from my earlier review ( |
ContactSection and FridgeClosingCta hard-coded their heading and error ids, so two on one page collided. Derive them with useId. A prose or grid section without a heading is no longer labelled by a missing id, and the grid no longer renders an empty h2. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
ctaSection has an optional heading, but FridgeClosingCta always rendered an empty h2 and bound aria-labelledby to it, which named the region with nothing. Make heading and note optional, render each only when present, and pass the section values straight through. Add a CTA case to the heading-ID test. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Signed-off-by: jay23606 <4602056+jay23606@users.noreply.github.com>
0148748 to
caf8791
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Bug
Repeated prose, split, and activity sections reused hardcoded heading IDs, so their
aria-labelledbyreferences could resolve to the wrong heading.Change
Generate a per-instance heading ID with Vue's
useId()and use it for each section's heading andaria-labelledbyreference.Testing
node node_modules/vitest/vitest.mjs run tests/sectionHeadingIds.test.tsgit diff --checkRegression test: fails on base twice (/expected 2 to be 1/), passes on fix twice (
node node_modules/vitest/vitest.mjs run tests/sectionHeadingIds.test.ts)Fixes #80