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 current item indicator for header navigation #1067

Open
wants to merge 23 commits into
base: header-breaking-changes
Choose a base branch
from

Conversation

frankieroberto
Copy link
Contributor

@frankieroberto frankieroberto commented Nov 6, 2024

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). 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:

Screenshot 2024-11-06 at 23 33 19

When displayed in the expanded menu view (eg on mobile), they have a blue border to their left instead, and are slightly indented:

Screenshot 2024-11-06 at 23 46 46

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

@frankieroberto
Copy link
Contributor Author

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?

@frankieroberto frankieroberto changed the base branch from main to header-breaking-changes November 7, 2024 15:35
@frankieroberto
Copy link
Contributor Author

@paulrobertlloyd @anandamaryon1 I've updated this now so that it's merging into header-breaking-changes rather than main. @paulrobertlloyd looks like you've updated the header to use box-shadow instead of border-bottom for the black line on focus? I've updated the PR to set that to grey instead of the border now.

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...

@davidhunter08
Copy link
Contributor

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

Screenshot 2024-11-08 at 16 25 44

BBC

Screenshot 2024-11-08 at 16 45 46

DAC

Screenshot 2024-11-08 at 16 46 17

W3.org

While in-text links usually need an underline to help people with low vision or color blindness distinguish them from the surrounding text, this is not needed for links in menus if the menu can be clearly identified as such.
https://www.w3.org/WAI/EO/Drafts/tutorials/navigation/styling/

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?

@frankieroberto
Copy link
Contributor Author

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?

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?

@anandamaryon1
Copy link
Collaborator

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.

@anandamaryon1
Copy link
Collaborator

@frankieroberto how were you expecting the focus style to work on a current link? Currently the styling for the indicator overrides the focus style:
image

Should it instead be overridden by the focus style, so it looks like this? i.e. same as other links.
image

Or some custom style? Not sure that it's important to see visually that you're focussed on the current link?

@frankieroberto
Copy link
Contributor Author

frankieroberto commented Nov 12, 2024

@anandamaryon1:

how were you expecting the focus style to work on a current link? Currently the styling for the indicator overrides the focus style:
image

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. 👍

@anandamaryon1
Copy link
Collaborator

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.

@frankieroberto
Copy link
Contributor Author

@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.

@anandamaryon1
Copy link
Collaborator

anandamaryon1 commented Nov 12, 2024

@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).

@paulrobertlloyd paulrobertlloyd force-pushed the add-header-navigation-current-item-indicator branch 2 times, most recently from ec40268 to afd52b5 Compare November 12, 2024 22:43
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.
@paulrobertlloyd paulrobertlloyd force-pushed the add-header-navigation-current-item-indicator branch from afd52b5 to 5cde630 Compare November 12, 2024 22:47
@paulrobertlloyd
Copy link
Contributor

paulrobertlloyd commented Nov 12, 2024

Note

This branch had gotten corrupted with changes from main, whereas it’s a branch of header-breaking-changes. Rebased header-breaking-changes with main, and then rebased this branch of header-breaking-changes. I then merged the 3 previous commits in this PR into one. Make sure you pull these changes from each branch before making further changes.

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:

Screenshot of header’s mobile menu.

@paulrobertlloyd
Copy link
Contributor

paulrobertlloyd commented Nov 12, 2024

I also updated the white header with navigation example to include an active item.

Here’s the active/current item style on the white header:

Screenshot of white header variant.

Which makes me wonder, should it also then be grey in the drop down menu, which shares the same colour palette (or conversely, should it be a blue border on the white header)?

@frankieroberto
Copy link
Contributor Author

This branch had gotten corrupted with changes from main, whereas it’s a branch of header-breaking-changes. Rebased header-breaking-changes with main, and then rebased this branch of header-breaking-changes. I then merged the 3 previous commits in this PR into one.

@paulrobertlloyd thanks for sorting this! 👏 Originally I’d thought it’d be separate from the other header stuff so branched from main but switching the base branch then confused things.

Here’s what I’ve changed it to, can of course change back if we want:

Screenshot of header’s mobile menu.

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:

Screenshot 2024-11-13 at 00 29 57

(Whether it needs border between each item or not I’m less sure on)

@frankieroberto frankieroberto marked this pull request as ready for review November 13, 2024 00:33
@edwardhorsford
Copy link
Contributor

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.

@anandamaryon1
Copy link
Collaborator

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.

@frankieroberto
Copy link
Contributor Author

@anandamaryon1 @paulrobertlloyd mobile current item indicator bumped up to 8px in ae3aaba:

4px 8px
Screenshot 2024-11-27 at 22 17 09 Screenshot 2024-11-27 at 22 16 45

Possibly a bit too close to the text but maybe it’s just about passable?

@frankieroberto
Copy link
Contributor Author

Current item indicator on the white header navigation variation changed from grey to blue in 74b72c8:

Grey Blue
Screenshot 2024-11-27 at 22 20 46 Screenshot 2024-11-27 at 22 20 27

@frankieroberto
Copy link
Contributor Author

...and just for good measure, here’s how it currently looks in the variant with the white header but the blue nav (which I only just discovered was an option):

Screenshot 2024-11-27 at 22 23 06

@paulrobertlloyd
Copy link
Contributor

Possibly a bit too close to the text but maybe it’s just about passable?

@frankieroberto What about 6px?

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.

5 participants