-
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
Add current item indicator for header navigation #1067
base: header-breaking-changes
Are you sure you want to change the base?
Add current item indicator for header navigation #1067
Conversation
I think this is a non-breaking change, and there aren’t any cross-dependencies on the header breaking change PR, so we could choose to either merge it in with those or ship it earlier as a minor release, depending on how things go? |
@paulrobertlloyd @anandamaryon1 I've updated this now so that it's merging into The 'current' item in the expanded menu is unchanged, using a blue box-shadow on the left. It does feel maybe a bit weird to use grey in one context and blue in the other, but then they're completely different designs so 🤷♂️? Possibly the expanded navigation would feel like more of a natural extension of the main navigation if it was also blue instead of white - but that'd be a much bigger change... |
Hey @frankieroberto, loving this work. I was just wondering do we know why the underlines were added to the nav items? The original nav didn't have underlines, I think they got added when the header was updated last year?! It feels a bit odd to me, and makes it a little harder to read, but maybe that's just me? Other websites don't tend do underline nav items. GOV.UK Design system BBC DAC W3.org
So it feels like if we have have evidence from users that underlines are needed on the main nav, then we should include them (and also feed that evidence back to gov.uk). But if we don't have any evidence, then maybe consider removing them again? |
Good question. I’d be up for removing the underlines on the links in the nav. Don't know if @anandamaryon1 has any history on this? |
Hmm I've had a look around and can't find anything definitive… I can see from some documentation that in June 2022 the header still had underlines. But in November 2020 the (nhs.uk website) header had no underlines. 🤔 I can try to do more digging and ask around. |
@frankieroberto how were you expecting the focus style to work on a current link? Currently the styling for the indicator overrides the focus style: Should it instead be overridden by the focus style, so it looks like this? i.e. same as other links. Or some custom style? Not sure that it's important to see visually that you're focussed on the current link? |
Opps, thought I’d checked this. Might have got un-done when I updated from the header breaking changes branch and had to switch from border-bottom to the box-shadow. Fixed in 860df16. 👍 |
Wonder whether we should add some guidance around this in the service manual. I notice that the GOV service nav doesn't, but I think it could be useful to give an example to show it's an option and explain that there are two different aria labels. Anything else we'd wanna cover? Nunjucks macros may be enough if not. |
@anandamaryon1 agree to having some good guidance around it - the Nunjucks macro options are often missed. What do you think to the thickness of the line at the bottom? I did wonder if it was too subtle and should be increased, but maybe it’s ok as is. Not quite sure how this PR also picked up the task list changes - will try and unpick that. |
@frankieroberto hmmI thought it was showing task list stuff because the header-breaking-changes branch was out of date with main so I updated it, but it's still showing them. Maybe swapping the base branch out and back again will fix it? I feel the underline thickness is enough as it is, since it's breaks up such a big strong shape (the blue header). |
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.
f5a7da9
to
08add40
Compare
ec40268
to
afd52b5
Compare
Add support for indicating the current section, using either `current: true` (if the user is currently on this exact page) or `active: true` (if the user is in that section but not necessarily that exact page). When displayed in the regular view within the blue header, the active links have a light-grey border at the bottom. When displayed in the expanded menu view (eg on mobile), they have a blue border to their left.
afd52b5
to
5cde630
Compare
Note This branch had gotten corrupted with changes from Given the above, also updated the comments to be a bit shorter and use 80 chars. Also updated the current/active state for the mobile menu, using the same 4px line weight, and removing the padding (I thought this was a bug until I later read your comment @frankieroberto). Sorry, this was very naughty of me, but I was in the branches and couldn’t help but tinker 😊 Here’s what I’ve changed it to, can of course change back if we want: |
@paulrobertlloyd thanks for sorting this! 👏 Originally I’d thought it’d be separate from the other header stuff so branched from
Looks good! I had thought it might be too tight to fit it in the margin but it seems ok. I’ve pushed a quick fix for focus style on the current item in the expanded menu, which was missing its black line at the bottom. See fd57863. I did also have a play with making the expanded menu use the same colours - ie white on blue with the grey indicator for the current item and it does at least feel more consistent and like an extension of the main nav to me: (Whether it needs border between each item or not I’m less sure on) |
Glad to see this being added. Stylistically I feel like the active item should have a larger grey border - it's very subtle right now. I also think we'd probably want to change its colour for the white header - it's currently far too low contrast on white. |
The white org header current indicator should be blue I think, the grey is too light. The mobile indicator at 4px and no indent gets a bit lost, particularly on my phone which has a slightly rounded bevel. Maybe 8px will work? Blue background for overflow menu, I'll have to take more of a look at, not sure about it. The logic makes sense, but it's a bigger change that probably should be separate from this PR. |
@anandamaryon1 @paulrobertlloyd mobile current item indicator bumped up to 8px in ae3aaba:
Possibly a bit too close to the text but maybe it’s just about passable? |
Current item indicator on the white header navigation variation changed from grey to blue in 74b72c8:
|
@frankieroberto What about 6px? |
9ab62ce
to
a1ee8c1
Compare
Add support for indicating the current section, using either
current: true
(if the user is currently on this exact page) oractive: true
(if the user is in that section but not necessarily that exact page). Both are styled the same but have different ARIA attributes.When displayed in the regular view within the blue header, the active links have a light-grey border at the bottom, and no text underline:
When displayed in the expanded menu view (eg on mobile), they have a blue border to their left instead, and are slightly indented:
In addition, a
<strong>
tag wraps the current item text, to give some indication in any scenarios where CSS is unavailable - based on the GOVUK service navigation component.Checklist