Skip to content

Conversation

@abourgoindev
Copy link
Contributor

@abourgoindev abourgoindev commented Jun 18, 2025

AccordionItem

  • 💥Replace header with h3 element
  • 💥Remove role="accordion-item"
  • ➕Add button element around h3's content
  • 🧪Add data-testid="accordion-button" to button element
  • 💥Move aria-expanded attribute to button element
  • 💥Move aria-controls attribute to button element
  • 🧪Add data-testid="accordion-label" to Typography element

@abourgoindev abourgoindev requested a review from ffffffelix June 18, 2025 15:41
@abourgoindev abourgoindev linked an issue Jun 18, 2025 that may be closed by this pull request
@abourgoindev
Copy link
Contributor Author

These modifications could have a big impact and cause a lot of breaking changes. This PR will be closed for now.

@abourgoindev abourgoindev reopened this Jul 18, 2025
@ffffffelix ffffffelix added patch Increment the patch version when merged minor Increment the minor version when merged and removed patch Increment the patch version when merged labels Jul 21, 2025
Copy link
Collaborator

@ffffffelix ffffffelix left a comment

Choose a reason for hiding this comment

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

  • As all interactions should be at the button level, we also have to update our styling to reflect that. All states style (:hover, :active, :focus-visible) should be on the button. Maybe we can refactor our CSS.

  • I propose to simplify data-testid label with accordion- instead of accordion-item- as the Accordion component is only a wrapper.

aria-expanded={open}
{...onAccessibleClick({onClick: e => handleClick(e, open)})}
role="accordion-item"
data-testid="accordion-item"
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Maybe we don't need to add a data-testid at this level, as there is no interaction or direct things to target
  • As ' aria-expanded` is at the button level, we should remove it from the heading.
  • Same for aria-controls that should be on the button. (W3C doc)

@abourgoindev
Copy link
Contributor Author

This PR will remain in draft until the next release, as the impact of the changes needs to be discussed with other teams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AccordionItem & W3C standards

3 participants