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: Make Panel heading optional #530

Merged
merged 5 commits into from
Jan 27, 2022
Merged

Conversation

dequejosie
Copy link
Contributor

@github-actions
Copy link
Contributor

Preview branch generated at https://panel-heading-optional.d1gko6en628vir.amplifyapp.com

Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

Heading can't be optional, Panel uses a section meaning it's a region and it must have a label.

<section
aria-labelledby={headingId}
className={classNames('Panel', className, {
['Panel--collapsed']: collapsed
})}
{...other}
>

For context, why is a heading not needed here?

@dequejosie
Copy link
Contributor Author

dequejosie commented Jan 25, 2022

@scurker It is for the User Management Table, which does not have a Panel level heading. Can we have a label without a visible heading for the Panel? Like when heading is not there, we require a label.

Screen Shot 2022-01-25 at 12 21 09 PM

@scurker
Copy link
Member

scurker commented Jan 25, 2022

@scurker It is for the User Management Table, which does not have a Panel level heading. Can we have a label without a visible heading for the Panel? Like when heading is not there, we require a label.

Is Manage Users not a heading in this instance? We can't make this change alone to make the heading optional because that would introduce an A11Y issue where the section has an invalid aria-labelledby.

If we want panel's heading to be optional, there's a couple of options

  • Make heading optional if an aria-labelledby attribute is passed into the component

OR

  • Make heading optional and only include auto aria-labelledby when the heading is present

Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

Just a few small nitpicks, and we need some tests to check for the conditional rendering of the heading.

It would also be nice to update docs.

@dequejosie dequejosie merged commit 49b3b90 into develop Jan 27, 2022
@dequejosie dequejosie deleted the panel-heading-optional branch January 27, 2022 01:33
@github-actions
Copy link
Contributor

Preview branch generated at https://panel-heading-optional.d1gko6en628vir.amplifyapp.com

@scurker scurker mentioned this pull request Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants