-
Notifications
You must be signed in to change notification settings - Fork 107
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
Update header navigation label #1073
base: header-breaking-changes
Are you sure you want to change the base?
Conversation
Removes the link to `"/"` labelled `"Home"`, which is currently hardcoded, and only shows up within the navigation menu on mobile screen widths. This link may not be appropriate for all services, which might not have a homepage, or might use a different path for it. It is also unclear whether having a homepage link is always needed in the navigation if the NHS logo also goes to the homepage.
@paulrobertlloyd I think we classify all HTML changes as breaking changes but I'm not 100% sure. Probably makes sense to merge this into the breaking-changes branch - then downstream users could make all the updates at once? Should we also make the aria 'Menu' attribute value configurable by params? (If only for any services doing translation into other languages) |
d6f4038
to
6c3cc4b
Compare
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.
Good to merge into the breaking changes branch. 👍
Should we also add a param for overriding the default / translation? Could be a follow-up PR?
With removing the |
Looking through the commit history, the <nav class="c-nav-primary" id="c-nav-primary" role="navigation" aria-label="Primary navigation" aria-labelledby="mainmenulabel"> Arguably you may want to add a skip link to the navigation (though we don’t provide that option or give any recommendations that you should). Maybe that is reason enough to keep it? |
9ab62ce
to
a1ee8c1
Compare
Description
The header navigation currently has an accessible label of ‘Primary navigation’; ‘navigation’ is superfluous, and will be read aloud as “Primary navigation navigation” by screen readers.
Updated this to ‘Menu’ as:
Also removed the
role
attribute as this role is inherent to thenav
element (duplicate roles were once needed, but that’s no longer the case).Finally, removed the
id
attribute, as don’t believe this is being used anywhere (we have a fewid
s in this component, that have removed in other PRs).Breaking change? Only if you are writing your own HTML. If we want it as part of the header breaking change, and I can rebase it onto the
header-breaking-changes
branch.Checklist