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

Add aria labelledby to alert notification banners #2020

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

vanitabarrett
Copy link
Contributor

@vanitabarrett vanitabarrett commented Nov 17, 2020

Related to #1954

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2020 November 17, 2020 16:00 Inactive
@vanitabarrett vanitabarrett changed the base branch from master to feature/notification-banner November 17, 2020 16:02
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2020 November 17, 2020 16:03 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2020 November 17, 2020 16:37 Inactive
NOTE: this PR does not update the macro documentation - this will be done separately in #2001

We have decided to keep the aria-labelledby attribute on alert banners too, to make things consistent with the error summary. We were seeing some odd behaviour when testing with NVDA and Voiceover, but this change did not seem to have a negative impact and it also means that when we next revisit the component we're at least working from a place where everything is consistent to start with.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2020 November 18, 2020 10:47 Inactive
@vanitabarrett vanitabarrett changed the title [TEST] Add aria labelledby to alert notification banners Add aria labelledby to alert notification banners Nov 18, 2020
@vanitabarrett vanitabarrett marked this pull request as ready for review November 18, 2020 10:48
@vanitabarrett vanitabarrett added this to the v3.10.0 milestone Nov 18, 2020
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Changes in 6db2e47 look good to me. Do we want to split 5322ecb into a separate PR?

@vanitabarrett
Copy link
Contributor Author

Good point @36degrees , I've raised a new PR here #2022

@vanitabarrett vanitabarrett merged commit 2af50d7 into feature/notification-banner Nov 18, 2020
@vanitabarrett vanitabarrett deleted the add-aria-labelledby branch November 18, 2020 12:51
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.

3 participants