-
Notifications
You must be signed in to change notification settings - Fork 9
NCRS-3933 add header with logo on same row as nav items #35
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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…does not exist on type JestMatchers' error in HeaderWithLogo test
…rror from header.js
|
alice-bell
commented
May 21, 2025
jamespickering5-nhs
approved these changes
May 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NCRS-3933 is a ticket to solve a problem with the text overlapping in the Masthead when the page is first loaded. This was causing failures in the integration tests, because the links get hidden behind the logo where they can't be clicked.
There is also a small PR in NCRS which includes further changes.
This branch is currently published to NPM as 2.1.0-test.4. To test the changes, you can pull my branch from the NCRS repo and run
yarn install && yarn serve
to play around with it locally.See also my comment on the NCRS-3933 ticket explaining why it's been done this way.
The problem
All the headers supported by nhsuk-frontend have the nav links on a separate row, where they use the full width of the screen. However, we have a layout with everything on one row, due to space constraints.
When the nav-bar uses only part of the width, it doesn't know how much space there is until the other components (e.g. logo) have loaded. This meant that when the page is refreshed at a small size, the nav items don't automatically move into a drop-down menu. As soon as the page is resized, even by one pixel, the layout is recalculated and everything snaps into place.
Screenshare.-.2025-03-11.1_48_20.PM.3.mp4
The solution
This pull request copies the various files necessary for the header component into nhsuk-react-components-extensions, so that the header style can be reused by other repos.
The changes to the code are tiny. The original code ran
calculateBreakpoints()
followed byupdateNavigation()
, to work out the widths of the items in the header and then push them into the drop-down menu if needed. The issue was thatcalculateBreakpoints()
didn't pick up the width of the logo on its first pass, so the logo ended up being overlapped by the nav items. All I've done is getcalculateBreakpoints()
andupdateNavigation()
to run twice, so that the second pass picks up the size of the logo and everything gets moved to the correct place.Code changes:
I've marked the important changes with comments. 90% of this work is an unchanged copy of existing code.
This sometimes gives a split-second view of the wrong layout before the page loads and it snaps into the right position. I can't think of a way to work around this (since the nav bar won't know the layout until the page has loaded, and therefore won't be able to put the items into the correct location) but I am open to suggestions if anyone can think of a workaround for this.
I've added a comment to the NCRS-3933 ticket, explaining why I've cloned all this into nhsuk-react-components-extensions instead of editing it at source.