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 secondary navigation #184

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Add secondary navigation #184

merged 1 commit into from
Dec 19, 2023

Conversation

frankieroberto
Copy link
Contributor

This adds a secondary navigation component, plus some documentation about it.

Resolves #182

Copy link
Contributor

@paulrobertlloyd paulrobertlloyd left a comment

Choose a reason for hiding this comment

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

Initial thoughts from a quick look.

x-govuk/components/secondary-navigation/template.njk Outdated Show resolved Hide resolved
docs/examples/secondary-navigation-with-header.njk Outdated Show resolved Hide resolved
docs/secondary-navigation.md Show resolved Hide resolved
@paulrobertlloyd paulrobertlloyd force-pushed the add-secondary-navigation branch 3 times, most recently from 18a11e5 to 3b5a7cf Compare December 16, 2023 00:48
@paulrobertlloyd
Copy link
Contributor

Have fixed the homepage layout, and rebased from the main branch where I updated linting on the project.

@frankieroberto
Copy link
Contributor Author

@paulrobertlloyd I've reworked the CSS on this (rather than the copy-and-paste job I had before), so now it's more similar to the primary navigation component implementation.

This means the same hover and focus styles are used as primary nav - ie with the double underline on hover, and the focus highlight being smaller. We can always review this separately if thinking on this changes.

This also means that the specific mobile view has been dropped, in favour of doing the same as the primary nav - ie horizontal with wrapping.

Let me know what you think!

@paulrobertlloyd paulrobertlloyd added this to the v3.0 milestone Dec 18, 2023
@frankieroberto
Copy link
Contributor Author

@paulrobertlloyd I've tweaked the padding so that it's reduced on mobile, which I think helps to make it clearer which item is the current page?

localhost_8080_examples_secondary-navigation_(iPhone 5_SE) (1)

In reality, I think for most services, the nav should fix on a single line for most mobile viewports (given how much wider phones are nowadays).

We could still consider an option to have the items stack vertically on mobile (with or without a toggle) - but I think if we do that, it's worth doing it for the primary nav and secondary nav at the same time?

Copy link
Contributor

@paulrobertlloyd paulrobertlloyd left a comment

Choose a reason for hiding this comment

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

Let’s leave thoughts of responsive versions of this for now; as you say, in most cases shouldn’t be needed and the items wrap much better now.

Also… do you think an example layout for the Prototype Kit would be a useful addition?

docs/examples/secondary-navigation-with-header.njk Outdated Show resolved Hide resolved
docs/examples/secondary-navigation-with-header.njk Outdated Show resolved Hide resolved
docs/examples/secondary-navigation.njk Outdated Show resolved Hide resolved
x-govuk/components/secondary-navigation/template.njk Outdated Show resolved Hide resolved
docs/secondary-navigation.md Show resolved Hide resolved
docs/examples/secondary-navigation.njk Outdated Show resolved Hide resolved
docs/secondary-navigation.md Outdated Show resolved Hide resolved
docs/secondary-navigation.md Outdated Show resolved Hide resolved
docs/secondary-navigation.md Outdated Show resolved Hide resolved
docs/secondary-navigation.md Outdated Show resolved Hide resolved
@paulrobertlloyd paulrobertlloyd force-pushed the add-secondary-navigation branch 3 times, most recently from 3300386 to 134d130 Compare December 19, 2023 19:58
@paulrobertlloyd paulrobertlloyd force-pushed the add-secondary-navigation branch from 134d130 to c042f3d Compare December 19, 2023 20:31
@paulrobertlloyd paulrobertlloyd self-requested a review December 19, 2023 20:36
@paulrobertlloyd paulrobertlloyd merged commit 2e14145 into main Dec 19, 2023
2 checks passed
@paulrobertlloyd paulrobertlloyd deleted the add-secondary-navigation branch December 19, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secondary navigation
2 participants