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 default aria label for navigation items #1946

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

owenatgov
Copy link
Contributor

What

Adds a default value for navigation_aria_label to the navigation_items sub-component.

Why

Currently, navigation_aria_label is passed directly to navigation_items without any defaulting under the assumption that it's only used by layout_header. This however breaks anything that's only using the separately exposed navigation_items. Adding a default here allows the sub-component to render without needed to pass a custom aria label if it isn't required.

No visual changes.

@bevanloon bevanloon temporarily deployed to govuk-publis-default-ar-xawdhv February 19, 2021 16:14 Inactive
@owenatgov owenatgov force-pushed the default-aria-label-for-navigation-items branch from ad4ef48 to f9dd6da Compare February 19, 2021 16:16
@bevanloon bevanloon temporarily deployed to govuk-publis-default-ar-xawdhv February 19, 2021 16:16 Inactive
@owenatgov owenatgov marked this pull request as ready for review February 19, 2021 16:16
Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@owenatgov owenatgov merged commit fcf9be7 into master Feb 24, 2021
@owenatgov owenatgov deleted the default-aria-label-for-navigation-items branch February 24, 2021 14:17
@@ -1,3 +1,5 @@
<% navigation_aria_label ||= "Top level" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm obviously late to this party, but it would be great if this text could be pulled from the translations file.

This was referenced Mar 11, 2021
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.

5 participants