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

Fix header navigation overflow on desktop #1060

Closed
wants to merge 13 commits into from

Conversation

paulrobertlloyd
Copy link
Contributor

@paulrobertlloyd paulrobertlloyd commented Oct 24, 2024

Description

Fixes issue where navigation list with too many items would not show the ‘Menu’ toggle on desktop, and items would overflow.

Checklist

frankieroberto and others added 11 commits October 23, 2024 19:22
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 paulrobertlloyd added 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) header labels Oct 24, 2024
@paulrobertlloyd paulrobertlloyd force-pushed the header-overflow-nav-items-desktop branch from b9e160e to 8c40c97 Compare October 24, 2024 16:36
@anandamaryon1
Copy link
Collaborator

anandamaryon1 commented Oct 31, 2024

Nice, logic seems to work well.

Style wise, at 990 width the links go centred and lose their dividers.

990px
image

989px
image

I think perhaps the 989px styling should be preserved at larger screens too? Thoughts? @paulrobertlloyd

Also looks like we need the page container max-width on the overflow menu items.

@paulrobertlloyd
Copy link
Contributor Author

I agree, and that styling is fixed in #1059. This PR fixes the broken JS only (and so can be included in the next minor release).

@anandamaryon1 anandamaryon1 changed the base branch from main to header-breaking-changes October 31, 2024 13:36
@anandamaryon1 anandamaryon1 changed the base branch from header-breaking-changes to main October 31, 2024 13:53
@paulrobertlloyd
Copy link
Contributor Author

Will create a new PR as this one now includes breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation) header
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants