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

Line up navigation items with header #999

Closed
wants to merge 15 commits into from

Conversation

frankieroberto
Copy link
Contributor

@frankieroberto frankieroberto commented Aug 20, 2024

Description

Whilst working on the 'Logged in header' (nhsuk/nhsuk-service-manual-community-backlog#192) with colleagues, we discussed how the navigation items don't currently align on the left with the NHS logo and the right with the end of the horizontal line, due to padding within the navigation link items.

This becomes more noticeable if additional items (such as account navigation and sign out links) are added to the header.

This could be resolved by adding some negative margin to the navigation container, so that the first and navigation items move into the gutter slightly, but because of the padding the text now lines up.

This does mean they are now out of alignment when focused, but this seems ok?

As a small extra bonus, this also buys 32 extra pixels of width for the navigation list.

This also means that on mobile, when the expanded menu is shown, the items in the expanded menu line up with the main menu.

Current Proposed
before after
before-focused afer-focused
Mobile before Mobile after

Checklist

Needed for absolute positioning of the drop-down nav
@frankieroberto frankieroberto marked this pull request as ready for review August 28, 2024 14:01
@frankieroberto frankieroberto changed the title line up navigation items with header Line up navigation items with header Aug 28, 2024
@anandamaryon1
Copy link
Collaborator

The mobile styling still needs a little tweak to make it line up properly.

Wondering about the best way to do this…

Options I can think of:

  • removing the left and right padding on the links completely (like the gov service nav) and using the nhsuk-navigation-container's 16px margins
  • increasing the padding on the nav links to 16px so they align (but this spaces them out slightly more than currently, 32px vs 24px)
  • adding back in a slight margin left to the nav container nhsuk-navigation to line it up (4px)

Any thoughts? @frankieroberto

I'm leaning towards the gov service nav approach now, as it makes it much simpler, no need to add negative margins or tweak anything for responsive.

@frankieroberto
Copy link
Contributor Author

@anandamaryon1 the first option (removing left and right padding on the links) means a slightly smaller target area, but does seem simpler, and also means the yellow focus state also aligns with the header. Is that ok, do you think?

Would still be 56 pixels high (or 64 pixels high on mobile), and should always be at least 44 pixels wide so long as the labels are around 4 characters or more. The shortest one on NHS.UK ("Health A-Z" would be 80px wide without horizontal padding).

@paulrobertlloyd
Copy link
Contributor

There is another option of course…

Screenshot 2024-09-27 at 10 51 45

Benefits of this approach being:

  • Consistent with focus state used for links elsewhere in the design system
  • Doesn’t obscure any current section indicators
  • Much less wrangling of CSS (just took a quick peak, and there’s a lot that could go away if this were simpler)

@frankieroberto
Copy link
Contributor Author

I had a quick go at another option here: #1033.

@paulrobertlloyd worth considering that too! It does potentially mean either reducing the height of the target area (which service navigation does) or keeping the taller target area but only having the focus yellow background colour behind the text rather than the full height.

I'm also not totally convinced that having both an underline on the focused item AND the current item is necessary, and it does look a bit odd to me, but maybe it's ok?

Screenshot 2024-09-27 at 10 00 34

@anandamaryon1
Copy link
Collaborator

Hmm, playing with the service nav example, I do think our increased target area is helpful, given that the links are within a navigation menu and will be clicked a lot. If we were to go with an option that reduces the visible focus state, I'd hope to maintain the biggest possible click area.

I don't think it's a big issue for the focus state to be misaligned, it only shows fleetingly in usage and the alignment of the default state has a bigger benefit.

Also, on the potential obscuring of a current page highlight, I don't think this is an issue, since on page load the focus state will not be showing, leaving any current page highlight visible (until a user focusses an item).

So, I now vote that we keep this as-is (as currently implemented in this PR).

@frankieroberto
Copy link
Contributor Author

@anandamaryon1 makes sense and I'd agree that bigger click areas areas are better!

I think I fixed the alignment on mobile on this PR, but definitely worth double-checking and doing some testing on different browsers.

Suggestions on the CSS also welcome - I'm still getting my head around how the CSS works for the header with all the different combinations of screen widths and number of nav items...

@anandamaryon1
Copy link
Collaborator

Hi @frankieroberto, doing some testing on this and found this little issue:

The changes in left and right margins have introduced this small gap (4px) on the drop down menu nhsuk-header__drop-down:
image

vs the current:
image

@anandamaryon1
Copy link
Collaborator

And another one, with the org header.

The logo, search, and nav items do not align with the horizontal divider line:
image

current:
image

@anandamaryon1
Copy link
Collaborator

Interestingly, the NHS App design system have done this already by stripping off the left and right padding, like #1033:
image
https://design-system.nhsapp.service.nhs.uk/

Certainly easier to implement…

Did you do this @davidhunter08, any thinking behind it or thoughts on how we should approach it for the main components?

@frankieroberto
Copy link
Contributor Author

@anandamaryon1 think I’ve fixed both issues now – the CSS is quite complicated though!

Comment on lines +535 to +540
padding: 12px 16px;

@include mq($from: desktop) {
padding-left: 32px;
padding-right: 32px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have something like this:

$header-navigation-link-padding-block: 12px;
$header-navigation-link-padding-inline: 16px;

Then use these variables elsewhere? That means only needing to change values in one place, and makes it clearer why negative margins are being used elsewhere.

frankieroberto and others added 2 commits October 18, 2024 09:46
Co-authored-by: Paul Robert Lloyd <[email protected]>
Co-authored-by: Paul Robert Lloyd <[email protected]>
@anandamaryon1
Copy link
Collaborator

Closing this one as we opted for approach in #1033

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants