Skip to content

Left align header navigation items by default #1138

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

Merged
merged 5 commits into from
Mar 13, 2025

Conversation

paulrobertlloyd
Copy link
Contributor

@paulrobertlloyd paulrobertlloyd commented Feb 14, 2025

Description

  • Left align items in the header navigation
  • Provide a new modifier class, .nhsuk-header__navigation-list--justified which restores the previous behaviour.
  • Remove the previous modifier class, .nhsuk-header__navigation-list--left-aligned, as this presentation is now the default.
  • Increase the space between navigation items from 24px to 32px above the desktop breakpoint, which is a little closer the previous design (items look a bit too closer together above this breakpoint without making this change)
  • Fixes Header responsive layout breaks with JS disabled #1093, navigation items not wrapping if JavaScript is not enabled
  • Fixes issue with menu button being shown too early or not early enough on smaller screens
  • Fixes issue with older browsers (namely Safari 14 device-locked to older iPads) where column-gap is not supported on flex containers. Items are now separated by inline padding on navigation list items

Checklist

@paulrobertlloyd paulrobertlloyd changed the title Header navigation spacing Left align header navigation items by default Feb 14, 2025
@paulrobertlloyd paulrobertlloyd force-pushed the header-navigation-spacing branch from 792369f to f6c5a8c Compare February 14, 2025 19:21
@paulrobertlloyd
Copy link
Contributor Author

FYI, visual regression tests are failing because I only added those related to this change; the text is the search input changed in a previous commit, which is causing the failures.

Copy link
Contributor

@frankieroberto frankieroberto left a comment

Choose a reason for hiding this comment

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

Love it.

@anandamaryon1
Copy link
Contributor

Thanks @paulrobertlloyd this is great, and bonus that it fixes #1093 !

Testing it, I find that items are being hidden behind the 'More' link earlier than they should? (Pretty sure there is enough room to not hide the last item)

Maybe a corresponding change to the JS needed?

This branch:
image

Header breaking branch (without this chnage):
image

@paulrobertlloyd paulrobertlloyd force-pushed the header-navigation-spacing branch from f6c5a8c to 0e61a66 Compare February 21, 2025 23:22
@paulrobertlloyd
Copy link
Contributor Author

paulrobertlloyd commented Feb 21, 2025

@anandamaryon1 Good catch! There was a simple fix to this, but the more I played around resizing the browser window, the more related issues I uncovered.

So, this PR now also addresses a number of additional issues relating to navigation items, namely showing the menu button correctly (not too early nor too late), occasionally showing more navigation items on smaller screens, and separates items by using padding instead of column-gap, meaning this part of the header should appear less broken in Safari 14/older iPads.

@paulrobertlloyd paulrobertlloyd force-pushed the header-navigation-spacing branch from 0e61a66 to 6f0c5bf Compare February 21, 2025 23:40
@anandamaryon1
Copy link
Contributor

@paulrobertlloyd
Thanks Paul, nice work.

Showing the Menu button depending on viewport width seems to be working nicely now.

Spacing older on Safari's also looking improved 👍
image

On the .nhsuk-header__navigation-list--justified class, is there a way to add that as a Nunjucks option? It seems currently that if you want a justified nav you'd have to use only HTML? (unless I'm missing something)

@paulrobertlloyd
Copy link
Contributor Author

On the .nhsuk-header__navigation-list--justified class, is there a way to add that as a Nunjucks option?

Depends how much we want to expose it as an option (the left-aligned option it replaces also wasn’t a Nunjucks parameter). I’d be happy leaving it as a documented class you can add to reinstate justified items, but maybe others have a view.

@frankieroberto
Copy link
Contributor

@paulrobertlloyd @anandamaryon1 I think it’s fine for it to be based on adding the extra modifier class, but maybe we should also add a way to add extra classes on the navigation list using the Nunjucks macro?

I think it’d just be a case of supporting params.primaryLinks.classes, which would be backwards-compatible (and might be useful anyway)?

@paulrobertlloyd
Copy link
Contributor Author

I think it’d just be a case of supporting params.primaryLinks.classes, which would be backwards-compatible (and might be useful anyway)?

I like this suggestion. Do we want to include support for params.primaryLinks.classes as part of this PR?

@anandamaryon1
Copy link
Contributor

Leave Nunjucks option for justified out, but leave justified class available.

.nhsuk-header__navigation-list {
.nhsuk-navigation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remember why this class name changed? I’d have expected all the css to be scoped within .nhsuk-header* but I might have forgotten something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven’t changed any class names, just changed which element these styles are applied to.

The classes used in this component are a bit all over the place (as elsewhere). For example, the header contains:

  • nhsuk-navigation-container
  • nhsuk-navigation
  • nhsuk-mobile-menu-container
  • nhsuk-organisation-*
  • nhsuk-logo
  • nhsuk-search__*

If we want to fix this, perhaps something for a different PR?

@paulrobertlloyd paulrobertlloyd force-pushed the header-navigation-spacing branch from 6f0c5bf to 55ac87e Compare March 13, 2025 21:36
@paulrobertlloyd paulrobertlloyd merged commit 423b70b into header-breaking-changes Mar 13, 2025
10 checks passed
@paulrobertlloyd paulrobertlloyd deleted the header-navigation-spacing branch March 13, 2025 21:42
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