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.

Sign up for GitHub

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

Jump to bottom

Update header styles so that .nhsuk-header__search-no-nav class is no longer needed #1046

Merged
merged 4 commits into from
Oct 23, 2024
Merged

Update header styles so that .nhsuk-header__search-no-nav class is no longer needed #1046

merged 4 commits into from
Oct 23, 2024

Conversation

paulrobertlloyd
Copy link
Contributor

@paulrobertlloyd paulrobertlloyd commented Oct 9, 2024 •
edited
Loading

Description

(Split out from #1000)

  • Update header styles so that .nhsuk-header__search-no-nav class is no longer needed when the header contains a search field but no navigation
  • Update header Nunjucks template so that it no longer repeats markup for search and navigation

This is not a breaking change 🙂

Checklist

@paulrobertlloyd paulrobertlloyd added header Code improvements Any changes that improve but do not alter the appearance of the site labels Oct 9, 2024
@paulrobertlloyd
Copy link
Contributor Author

Oooh, the backstop regression testing is super helpful! Caught a bug, and also turns out this PR fixes a few other issues:

  • Corrects the bottom padding of the header when there’s search but no navigation
  • Removes the navigation border that was showing when there were no navigation items

That explains some of the updated backstop images. The remaining changes are because:

  • The bottom padding is now equal to the top padding
  • Subtle movement of navigation items because the navigation border is now placed on the top of the navigation list, not the bottom of the header container

frankieroberto
frankieroberto previously approved these changes Oct 15, 2024
View reviewed changes
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.

1 minor suggestion

packages/components/header/_header-transactional.scss Outdated Show resolved Hide resolved
@paulrobertlloyd
Copy link
Contributor Author

@anandamaryon1 If happy with the changes, this is ready to merge.

frankieroberto
frankieroberto approved these changes Oct 23, 2024
View reviewed changes
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.

Nice! Code is simpler and the minor changes in the reference images all look like improvements to me! 🙌

@paulrobertlloyd paulrobertlloyd merged commit 946a655 into main Oct 23, 2024
2 checks passed
@paulrobertlloyd paulrobertlloyd deleted the header-search-no-nav branch October 23, 2024 10:44
@paulrobertlloyd paulrobertlloyd mentioned this pull request Oct 23, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anandamaryon1 anandamaryon1 anandamaryon1 left review comments

@frankieroberto frankieroberto frankieroberto approved these changes

@lasara-d lasara-d Awaiting requested review from lasara-d

Assignees
No one assigned
Labels
Code improvements Any changes that improve but do not alter the appearance of the site header
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulrobertlloyd @frankieroberto @anandamaryon1