-
Notifications
You must be signed in to change notification settings - Fork 204
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
[IRN-5693][BpkModal] Support dialog a11y label when no header #3767
Conversation
Visit https://backpack.github.io/storybook-prs/3767 to see this build running in a browser. |
Browser supportIf this is a visual change, make sure you've tested it in multiple browsers. |
@@ -125,6 +127,7 @@ const BpkModalInner = ({ | |||
tabIndex={-1} | |||
role="dialog" | |||
aria-labelledby={showHeader ? headingId : undefined} | |||
aria-label={!showHeader ? ariaLabel : undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could skip this guard but seems sensible to protect the consumer from providing a title and aria label
(also considered using title
as guard but showHeader is more consistent with other guards)
@@ -62,6 +63,7 @@ export type ModalStyle = (typeof MODAL_STYLING)[keyof typeof MODAL_STYLING]; | |||
|
|||
const BpkModalInner = ({ | |||
accessoryView = null, | |||
ariaLabel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considered piggy-backing on the existing title
prop since not used in a "no header" scenario this feels cleaner and simpler from consumer PoV (and not much extra code logic on our part)
@@ -230,8 +230,7 @@ const NestedExample = (isOpen: boolean) => ( | |||
|
|||
const NoHeaderExample = (isOpen: boolean) => ( | |||
<ModalContainer | |||
title="Modal title" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing demo passed 2 unused fields - removed
@@ -230,8 +230,7 @@ const NestedExample = (isOpen: boolean) => ( | |||
|
|||
const NoHeaderExample = (isOpen: boolean) => ( | |||
<ModalContainer | |||
title="Modal title" | |||
closeLabel="Close modal" | |||
ariaLabel="Modal title" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use new field in "no header" demo
); | ||
|
||
expect(screen.queryAllByRole('heading')).toHaveLength(0); | ||
expect(screen.getByRole('dialog')).toHaveAttribute('aria-label', 'Modal title'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test new logic
); | ||
|
||
expect(screen.getByRole('heading')).toHaveTextContent('Actual title'); | ||
expect(screen.getByRole('dialog')).toHaveAccessibleName('Actual title'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify favours title if aria label also provided when not required
Visit https://backpack.github.io/storybook-prs/3767 to see this build running in a browser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! Thanks @steviehailey-skyscanner
Screenshots
No visual change
a11y DOM Before - role dialog with no a11y label
a11y DOM After - role dialog now has a11y label
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md