Refactor header #982
Merged
Refactor header #982
Conversation
This file contains 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
duncanjbrown
force-pushed
the
refactor-header
branch
from
3963092
to
d1828fa
Compare
|
Added a commit to include yarn in the GitHub Actions build as unit tests touching components depend upon it, or at least they do now |
This component brings together the logic for constructing header navigation for the various parts of the application. Instead of putting links into partials, header navigations are now configured by NavigationItems.for_candidate_interface, .for_provider_interface etc. These methods return lists of NavigationItems which the HeaderComponent knows how to turn into links. This means that no logic about what should be in navigations lives in templates. It exposes some wrinkles about the way we handle eg SupportUsers vs DfE Sign-in users in the same way across Support and Provider interfaces and should make refactoring these easier. This resolves a bug where the dropdown menu was appearing on a screen where it shouldn't, and hopefully prevents that kind of bug in the future. We also get to throw away a lot of repeated markup which risked being brittle.
Bring the rest of the <header> inside HeaderComponent. Add a classes: attr in line with the convention established by govuk-frontend, and use the appropriate HostingEnvironment to set it.
Components need frontend assets to render
|
Have added review comments with minor nits, then fixed them up + force pushed into the relevant commit. |
tvararu
force-pushed
the
refactor-header
branch
from
e28e3ac
to
fe80078
Compare
|
Took a good look at this on the review app, looks great! |
4 tasks
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.
Context
We (@duncanjbrown and I) made this change because the same HTML for the header is used across candidate, provider, support and API doc interfaces.
There was a lot of repeated code, and logic in the view that's better isolated outside of the Header component itself.
This repeated code caused a bug whereby the mobile toggle menu button appeared even when there were no menu items to show/hide.
Changes proposed in this pull request
We proposed abstracting the header and navigation items into a component that closely resembles the GOV.UK Frontend Header component.
Link to Trello card
https://trello.com/c/hKy03E02/691-create-header-component