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

Use section for emergency banner #2973

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

patrickpatrickpatrick
Copy link
Contributor

@patrickpatrickpatrick patrickpatrickpatrick commented Sep 20, 2022

What

Small adjustments to the template of emergency banner for accessibility reasons.

Why

When the emergency banner was deployed on GOV.UK, some changes were made based on the feedback from an accessibility specialist. These changes involved removing the role attribute from the parent div and changing the element of the parent div to a section. This is to avoid any accidental duplication of roles on any page of GOV.UK. In accordance with this change, the aria-label has been changed to aria-labelledby instead.

Co-author with @maxf who made the changes to static (which will be removed when the emergency banner static PR has been merged).

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2973 September 20, 2022 14:03 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2973 September 20, 2022 14:04 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2973 September 20, 2022 14:06 Inactive
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

I've compared the outputted markup against the banner that was deployed on production and the section tag and aria-labelledby linked with id match what was published there 👍

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Apologies for revoking the approval but I've just realised that it'd be good to add a test for the aria-labelledby to check it matches the id on the heading.

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2973 September 30, 2022 11:20 Inactive
@patrickpatrickpatrick
Copy link
Contributor Author

have added that test

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

LGTM 👍

When the emergency banner was deployed on GOV.UK, some changes were made
based on the feedback from an accessibility specialist. These changes
involved removing the role attribute from the parent div and changing
the element of the parent div to a section. This is to avoid any
accidental duplication of roles on any page of GOV.UK. In accordance
with this change, the aria-label has been changed to aria-labelledby
instead. Have also added a test to make sure the label is being applied
correctly.

Co-authored-by: Max Froumentin <[email protected]>
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2973 October 3, 2022 14:21 Inactive
@patrickpatrickpatrick patrickpatrickpatrick merged commit c84291c into main Oct 3, 2022
@patrickpatrickpatrick patrickpatrickpatrick deleted the update-emergency-banner branch October 3, 2022 14:26
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.

3 participants