RFC: Combine service and transactionalService header options and remove layout modifiers
#1000
Conversation
paulrobertlloyd
requested review from
frankieroberto,
anandamaryon1 and
lasara-d
…ader layout switches
paulrobertlloyd
force-pushed
the
refactor-header-component-organisation-and-service
branch
from
ce2ca3f
to
364e92e
Compare
|
This looks great @paulrobertlloyd. Nice work integrating the service name wrapping and the bonus feature of logo focus styles. I suppose the most controversial part is removing the transactional (smaller logo) variant, so I'll try get some thoughts/reviews from others on this too. Happy for you to split out some of these a smaller non-breaking PRs, if you can, but also happy to wait and release all of this together with the logged in header. Note to self: It'll need corresponding service manual guidance updates. |
| } | ||
| .nhsuk-header__service-link { | ||
| @include nhsuk-font(19, $line-height: 22px); | ||
| text-wrap: balance; |
There was a problem hiding this comment.
Can I suggest something a bit ugly?
If we add
margin-top: -2px; margin-bottom: -3px;
to this class.
Then the logo won't be pulled down slightly when the service name wraps. (it's only a couple pixels so perhaps not worth this ugly code?)
There was a problem hiding this comment.
I like this approach! The difference between the "service" and "transactional service" is visually very small, and it's not clear what the difference between those 2 types of services is.
One other thing to consider is whether or not there should be an option to have a single link which wraps both the logo and the service name, like the current header with service name?
You could link both the NHS logo and the service name to the same page separately, but that’s probably not ideal accessibility-wise?
I don’t think there's any clear rule about when services should link the NHS logo to the NHS homepage vs their service homepage, but I imagine that one might make more sense for patient-facing services and the other for staff-facing ones (although might not be a 1:1 match).
| @@ -32,138 +27,60 @@ | |||
| {%- endif %} | |||
| </a> | |||
| {%- else -%} | |||
| <a class="nhsuk-header__link{% if params.service %} nhsuk-header__link--service {% endif %}" href="{% if params.homeHref %}{{ params.homeHref }}{% else %}/{% endif %}" aria-label="{% if params.ariaLabel %}{{ params.ariaLabel }}{% else %}NHS homepage{% endif %}"> | |||
| <a class="nhsuk-header__link" href="{% if params.homeHref %}{{ params.homeHref }}{% else %}/{% endif %}" aria-label="{% if params.ariaLabel %}{{ params.ariaLabel }}{% else %}NHS homepage{% endif %}"> | |||
There was a problem hiding this comment.
Rather than defaulting to /, I wonder if this should not link at all if no params.homeHref is present? Or alternatively it could link to https://www.nhs.uk but perhaps having no link at all would be safer, to avoid it being overlooked?
Or I guess a third option would be to display a visible error message to force a link to be specified?
| </div> | ||
|
|
||
| {%- if params.service.name %} | ||
| <a class="nhsuk-header__service-link" href="{{ params.service.href | default('/') }}">{{ params.service.name }}</a> |
There was a problem hiding this comment.
Defaulting to / feels safer here (should generally work for most services!) but again it might be worth having an option for no link at all, especially as the service name doesn't visually look like a link, and perhaps for some simple transactional services, a link isn't needed?
There was a problem hiding this comment.
Possibly one for a follow-up, but perhaps it would now make sense to recombine all the header CSS into a single file?
Follow up to #996 and fixes #997
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 by refactoring the different variants and reducing the number of different layout variants and associated modifiers in the markup and CSS.
Note
In general, the header component appears to have suffered from a sticking plaster approach to adding features, accumulating tactical fixes that over time have increased the complexity of the Nunjucks template, its public API, the markup and CSS.
Before adding account links, we should aim to simplify and fix what is already here.
This PR:
transactionalboolean option and thetransactionalServiceoption. The component API now acceptsorganisationandserviceoptions, these being the 2 significant variants of the header. This means there are now only 2 sizes of NHS logo, of which only the organisation variant uses the smaller size..nhsuk-header__logo--only.nhsuk-header__transactional--logo.nhsuk-header__link--service.nhsuk-header__search-no-nav.nhsuk-header__content#content-headercontainer.nhsuk-header__transactional-service-name.nhsuk-header__transactional-service-name--link.nhsuk-header__service-linkstyleSome of these changes can (and probably should) be pulled out into smaller, non-breaking PRs, if required.
Handling long service names
Here’s a preview of a header with both navigation, search and a long service name, which addresses #997:
280px
280px (organisation)
400px
640px
800px
800px (organisation)
1024px
I have added the
text-wrap: balanceto.nhsuk-header__service-link; this is a relatively new CSS value, but can be used as a progressive enhancement; the screenshots above show it being used, but if a browser doesn’t support this value, a (long) service name will break later in its title.Logo focus styles
Checklist