Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[material-ui][Accordion] Render a heading wrapping AccordionSummary button per W3C Accordion Pattern standards #42914

Merged
merged 19 commits into from
Jul 27, 2024
Merged

[material-ui][Accordion] Render a heading wrapping AccordionSummary button per W3C Accordion Pattern standards #42914

merged 19 commits into from
Jul 27, 2024

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Jul 12, 2024 •
edited
Loading

Fixes #42891

Following the W3C Accordion Pattern, screen reader now announces correctly as:

button collapsed/expanded heading level 3

Previously, it announced as:

button collapsed/expanded

Docs previews:

Breaking change: https://deploy-preview-42914--material-ui.netlify.app/material-ui/migration/migrating-to-v6/#accordion

IMO, this change can be cherry-picked to v5 as it is not a breaking change.

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work accessibility a11y component: accordion This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material needs cherry-pick The PR should be cherry-picked to master after merge labels Jul 12, 2024
@mui-bot
Copy link

mui-bot commented Jul 12, 2024 •
edited
Loading

Netlify deploy preview

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 1f4eaf5

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Accordion] Render AccordionSummary as a heading wrapping a button per W3C Accordion Pattern standards [material-ui][Accordion] Render a heading wrapping AccordionSummary button per W3C Accordion Pattern standards Jul 12, 2024
@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review July 12, 2024 10:54
@ZeeshanTamboli ZeeshanTamboli mentioned this pull request Jul 12, 2024
Closed
DiegoAndai
DiegoAndai reviewed Jul 24, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @ZeeshanTamboli, sorry for the late reply.

This looks like a good addition 👍🏼

This might be considered a breaking change as it can break customizations relying on dom structure / specificity. What do you think @aarongarciah?

packages/mui-material/src/Accordion/Accordion.js Outdated Show resolved Hide resolved
@aarongarciah
Copy link
Member

@DiegoAndai yes, sadly, as of today, changes to the DOM structure of a component should be considered a breaking change.

@ZeeshanTamboli ZeeshanTamboli removed the needs cherry-pick The PR should be cherry-picked to master after merge label Jul 25, 2024
ZeeshanTamboli
ZeeshanTamboli commented Jul 26, 2024
View reviewed changes
pnpm-lock.yaml Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to run pnpm dedupe after merging with the latest next branch.

@aarongarciah
Copy link
Member

Since we consider this a breaking change, we need to add an entry in the v6 upgrade guide.

@ZeeshanTamboli
Copy link
Member Author

Since we consider this a breaking change, we need to add an entry in the v6 upgrade guide.

Added. https://deploy-preview-42914--material-ui.netlify.app/material-ui/migration/migrating-to-v6/#accordion.

aarongarciah
aarongarciah reviewed Jul 26, 2024
View reviewed changes
Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Looks good. I'll leave the final approval to @DiegoAndai

docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
DiegoAndai
DiegoAndai approved these changes Jul 26, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks @ZeeshanTamboli

@ZeeshanTamboli ZeeshanTamboli merged commit d69604d into mui:next Jul 27, 2024
22 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the issue-42891-accordion-heading branch July 27, 2024 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongarciah aarongarciah aarongarciah left review comments

@DiegoAndai DiegoAndai DiegoAndai approved these changes

@jcorradino jcorradino Awaiting requested review from jcorradino

Assignees
No one assigned
Labels
accessibility a11y breaking change bug 🐛 Something doesn't work component: accordion This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Milestone
No milestone
5 participants
@ZeeshanTamboli @mui-bot @aarongarciah @jcorradino @DiegoAndai