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

Remove horizontal padding within primary nav #157

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

frankieroberto
Copy link
Contributor

This removes the horizontal padding within the items in the primary navigation.

Whilst the horizontal padding does increase the target area, it also leads to the border-bottom on the first item extending into the margin of the page, which arguably looks a bit odd.

The draft One Login service header does not add horizontal padding, and neither does many of the service using this pattern (for example, Apply for teacher training) or the version of the pattern in the MOJ design system.

However the primary navigation bar on the Design System website itself does add horizontal padding.

An in-between option might be to have the padding for the increased target area, but to keep the border on the bottom of the active item to the width of the text.

However, given the height of the navigation bar, and the likely width of each item (which depends on the text), I think the target area size could already be considered sufficient?

Screenshots

Before

Screenshot 2023-10-31 at 13 02 52

After

Screenshot 2023-10-31 at 13 03 06

@paulrobertlloyd
Copy link
Contributor

paulrobertlloyd commented Oct 31, 2023

Looks good to me. The padding is such as it is because it follows the design of the Design System website, but given that the One Login service header has diverged, and consensus has formed around the alternative design, I think we should use that.

Feel free to merge after fixing the linting error.

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.

This line is tripping up the linter.

Also… oooh, new textboxes on GitHub!

@frankieroberto frankieroberto merged commit c492001 into main Nov 1, 2023
1 check passed
@frankieroberto frankieroberto deleted the remove-horizontal-padding-within-primary-nav branch November 1, 2023 11:58
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.

2 participants