Skip to content

fix: make collapsible section title a toggle button#2212

Closed
thegreatalxx wants to merge 5 commits intonpmx-dev:mainfrom
thegreatalxx:fix/collapsible-section-title-toggle
Closed

fix: make collapsible section title a toggle button#2212
thegreatalxx wants to merge 5 commits intonpmx-dev:mainfrom
thegreatalxx:fix/collapsible-section-title-toggle

Conversation

@thegreatalxx
Copy link
Copy Markdown

🔗 Linked issue

resolves #1518
resolves #1392

🧭 Context

The CollapsibleSection title 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 missing aria-expanded and aria-controls attributes required by WCAG for disclosure buttons.

📚 Description

  • Made the title text a <button> element that calls toggle() on click, so the full heading text is a clickable toggle target.
  • Added aria-expanded (bound to isOpen) and aria-controls (pointing to the content div id) to the title button.
  • Removed the redundant aria-label from the title button so the button's accessible name comes from its visible text content, avoiding a label/content mismatch.
  • Removed unused LinkBase import.

Tests added to test/nuxt/a11y.spec.ts verify that:

  1. Clicking the title button toggles aria-expanded from true to false and back.
  2. aria-controls references the actual content element id.
  3. The component passes the axe audit with a subtitle (which was a potential label mismatch source).

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Mar 22, 2026 7:04pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Mar 22, 2026 7:04pm
npmx-lunaria Ignored Ignored Mar 22, 2026 7:04pm

Request Review

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 22, 2026

📝 Walkthrough

Walkthrough

The CollapsibleSection.vue component has been refactored to remove the LinkBase dependency and replace the link-based title with a dedicated button element. The title button now uses aria-expanded and aria-controls attributes to trigger the existing toggle handler on click. This removes hash-navigation behaviour from the title area. Three new test cases verify the button's aria-expanded toggling behaviour, aria-controls reference integrity, and accessibility compliance using an axe audit.

Suggested reviewers

  • danielroe
  • knowler
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description clearly relates to the changeset, detailing the replacement of a link-based title with a toggle button and the addition of WCAG accessibility attributes.
Linked Issues check ✅ Passed The pull request meets requirements from both linked issues: #1518 is resolved by replacing hash-link navigation with a toggle button, eliminating scroll behaviour; #1392 is resolved by making the title text clickable for expand/collapse functionality.
Out of Scope Changes check ✅ Passed All changes are within scope: the CollapsibleSection component modification, LinkBase import removal, and three new accessibility test cases directly address the stated objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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-visible utility 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-element focus-visible utility classes.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 32f68e92-4a05-420a-911a-f97b8ec3de08

📥 Commits

Reviewing files that changed from the base of the PR and between 7f2fc1a and bc48f80.

📒 Files selected for processing (2)
  • app/components/CollapsibleSection.vue
  • test/nuxt/a11y.spec.ts

@knowler knowler self-requested a review March 22, 2026 22:56
Copy link
Copy Markdown
Member

@knowler knowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@howwohmm howwohmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solid a11y improvement — `` with `aria-expanded` and `aria-controls` is the correct semantic for a disclosure toggle, and the focus ring styling is good.

one thing: after this change, the heading has two buttons that both toggle the same content region — the existing chevron button (which already has `aria-expanded` and `aria-controls`) and the new title button. screen reader users navigating by buttons will encounter two separate toggle controls for the same collapsible section, which is confusing.

the fix would be to make the chevron purely decorative (`aria-hidden="true" tabindex="-1"`) since the title button now handles the toggle semantics. or alternatively, wrap both the chevron and title in a single `

` element.

(note: the axe audit passes because axe doesn't have a rule against duplicate `aria-controls` targeting the same element — this is more of a WCAG best-practice concern than a strict violation.)

@knowler
Copy link
Copy Markdown
Member

knowler commented Mar 24, 2026

Hey, @howwohmm and @TheAlxLabs thank you for contributing!

We’ve noticed a high volume of activity from both of your accounts and we just wanted to check in to make sure you understand our contribution guidelines with regard to AI usage. While we do accept code that is generated by AI, we prefer that you use your own words when writing comments (including reviews), issues, and pull request descriptions. In all cases, AI usage should reflect your own thinking and you should be able to take responsibility for it. So, please do not make claims that do not reflect your own expertise whether in PR descriptions or reviews. AI usage is not a substitute for thinking.

Do not reply to me here as I’d like to avoid taking this PR further off topic. Just give this comment a thumbs up if you’ve reviewed the relevant contributing guidelines and understood my message. If you’d like to discuss more, please reach out to me on Discord.

@MatteoGabriele
Copy link
Copy Markdown
Member

Closing this PR.

You've opened 14 PRs in a matter of minutes, which is a clear sign of automated bulk submissions with no genuine engagement.

While AI tools can be helpful for coding assistance, our project requires genuine human involvement. We've outlined this clearly in our contribution guidelines.

If you're genuinely interested in contributing to npmx.dev:

  • Engage with the community first (issues, discussions)
  • Understand the codebase and what we actually need
  • Submit thoughtful, individual PRs with your own descriptions
  • Follow up on feedback and participate in reviews
  • Use AI as a tool to assist you, not replace you

Mass-generated PRs like these won't be accepted. We welcome real contributions from developers who want to be part of our community.

@MatteoGabriele MatteoGabriele added the 007 This PR *may* not follow our code of conduct regarding AI usage. label Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

007 This PR *may* not follow our code of conduct regarding AI usage.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package page scrolls down when clicking side bar summary titles Clicking on sidebar headings should expand/collapse their section

4 participants