-
Notifications
You must be signed in to change notification settings - Fork 107
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
Simplify template logic for header component #996
Simplify template logic for header component #996
Conversation
caca0ad
to
8f4212a
Compare
All checks now pass; I hadn’t updated the |
79f7e96
to
d9a8437
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Think it just needs an entry in the CHANGELOG, and an update to packages/components/header/README.md
.
There's also a couple of uses of showNav
and showSearch
within app/
to update.
Not sure when the next breaking change release is planned for? @anandamaryon1
d9a8437
to
0343337
Compare
Updated the component Not sure this should be part of this PR, but I’d love to simplify the component API further by renaming I can create the corresponding PR on the service manual when we are happy with the shape of this PR, and if there any other changes we want to make to the header component at the same time. |
0343337
to
792b656
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'm not sure what our merge strategy should be here, as this would have to go into a breaking change release. @anandamaryon1?
Given this is already a breaking change, I'd be up for renaming primaryLinks to navigation and merging service and transactionService options as part of a follow-up PR targeting the same release?
@paulrobertlloyd I want to bring this together with the other header related breaking changes, but there are merge conflicts that are a bit complex, would you be able to take a look at merging in main first? |
Yes, happy to. A few things in this PR have happened on other PRs now, so this should be a simpler change once rebased. |
792b656
to
6d9644d
Compare
Rebased with |
135b9dc
to
d98430e
Compare
a83b671
to
6bd61ca
Compare
abb92ca
to
831a3a4
Compare
6bd61ca
to
623fc3e
Compare
1d7d142
to
9cf2141
Compare
623fc3e
to
5f618bb
Compare
5f618bb
to
83c96ce
Compare
Description
Prior to any work on adding account links to the header, or refining its design, this PR aims to reduce the complexity of the component template, and remove repetition of navigation and search markup.
This PR:
params.primaryLinks
andparams.search
respectivelydefault
filter for values as opposed to longerif
/else
tagssearch-field
toq
, to match that used by the NHS.UK website (all the other fallback values do this, so this should as well)As this PR stands, this is a breaking change, although only in terms of the Nunjucks macro options. Here are the changes:
showNav
primaryLinks
option providedshowSearch
search
option providedtransactional
transactionService
option providedsearch
search.placeholder
search.visuallyHiddenButton
search.visuallyHiddenLabel
searchAction
search.action
. Defaults tohttps://www.nhs.uk/search/
searchInputName
search.name
. Defaults toq
It should be noted that the service manual website, as just one example, currently has the label ‘Search the NHS website’, when in fact you can only search the service manual website. The new
search{}.visuallyHiddenLabel
option would make it possible to use a correct label.If this PR is accepted, a separate PR can be made against the service manual to update these options.
There’s a whole load of other refinements that could be made in terms of markup, styles and the different options provided, but I’ve tried to keep this change tightly scoped to focus only on reducing the complexity of this component’s templating logic, with the side effect being a reduction in the number of interconnected macro options provided to authors.
Why remove
showNav
,showSearch
andtransactional
options?While it could be suggested that these options make it easy to show/hide parts of the header, this can be achieved with Nunjucks in other ways. For example, currently you might do the following:
This could be written thus:
It can also get tricky and complex if you have to manage the state of the header across multiple options. The changes in this PR hopefully make it easier to manage this.
Checklist