fix: make collapsible section title a toggle button#2212
fix: make collapsible section title a toggle button#2212thealxlabs wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/CollapsibleSection.vue (1)
106-110: Use the project-wide focus-visible styling for this new button.The new title toggle button adds an inline
focus-visibleutility class. In this codebase, button focus-visible styles are handled globally, so this should be removed to keep styling consistent.♻️ Proposed change
- class="text-start focus-visible:outline-accent/70 rounded" + class="text-start rounded"Based on learnings: In the npmx.dev project, focus-visible styling for buttons and selects is applied globally in
app/assets/main.css; components should not add per-elementfocus-visibleutility classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32f68e92-4a05-420a-911a-f97b8ec3de08
📒 Files selected for processing (2)
app/components/CollapsibleSection.vuetest/nuxt/a11y.spec.ts
knowler
left a comment
There was a problem hiding this comment.
Instead of creating a new separate button next to an existing button, we should just use a single button for the expand/collapse functionality, as having two is verbose and extra work to get past when using keyboard nav or a screen reader. It should include both the chevron icon and the section title.
Further, similar to what you’ve done with this new button, we should get rid of the computed aria-label as toggle buttons shouldn’t change their label based on state as the state is already communicated with aria-expanded.
Finally, we should keep the links to the sections. I was thinking that it can still use the chain link icon without any visible text, but the format of the accessible name would be Permalink for [section title].
Let me know if you’ve got any questions.
🔗 Linked issue
resolves #1518
resolves #1392
🧭 Context
The
CollapsibleSectiontitle text was not interactive — only the small chevron button beside it toggled the section. This meant users had to click a small icon rather than the larger, more obvious heading text. Additionally, the title button was missingaria-expandedandaria-controlsattributes required by WCAG for disclosure buttons.📚 Description
<button>element that callstoggle()on click, so the full heading text is a clickable toggle target.aria-expanded(bound toisOpen) andaria-controls(pointing to the content div id) to the title button.aria-labelfrom the title button so the button's accessible name comes from its visible text content, avoiding a label/content mismatch.LinkBaseimport.Tests added to
test/nuxt/a11y.spec.tsverify that:aria-expandedfrom true to false and back.aria-controlsreferences the actual content element id.