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.

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

fix(accordion): add heading role and aria level to title #1616

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mivaylo
Copy link
Contributor

@mivaylo mivaylo commented Nov 12, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Accordion and Stepper headers don't have role and aria-level assigned.

Issue Number: CDE-2437

What is the new behavior?

Accordion and Stepper headers have role=heading and aria-level can be assigned from the user wit hdefaul value of 3.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mivaylo mivaylo self-assigned this Nov 12, 2024
@mivaylo mivaylo marked this pull request as draft November 12, 2024 18:32
Copy link
Contributor

github-actions bot commented Nov 12, 2024

👋 @valentin-mladenov,

  • 😭 The build for this PR has failed
  • 🗒 Please check the build log
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal Clarity Support space

Thank you,

🤖 Clarity Release Bot

@valentin-mladenov valentin-mladenov marked this pull request as ready for review December 3, 2024 12:28
@valentin-mladenov valentin-mladenov requested a review from a team December 3, 2024 12:31
@valentin-mladenov valentin-mladenov changed the title fix(accordion): add heading role to title fix(accordion): add heading role and aria level to title Dec 3, 2024
Copy link
Contributor

@dtsanevmw dtsanevmw left a comment

Choose a reason for hiding this comment

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

Check the implementation of stack-view where we are checking if the component is nested so we put 4 instead of 3 for the default value

@kevinbuhmann
Copy link
Member

kevinbuhmann commented Dec 3, 2024

This is a breaking change because it can cause pages with legal heading structure to suddenly have illegal heading structure due to the new headings.

This can be fixed by not setting accordion titles as heading by default to maintain current heading structures. This should be an opt-in feature until v19.

I have added the breaking change label due to the current implementation. If this issue is fixed, that can be removed.

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

Successfully merging this pull request may close these issues.

5 participants