-
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
Show account information and links in the header #1063
base: header-breaking-changes
Are you sure you want to change the base?
Conversation
55a55f7
to
aa6eb5a
Compare
@paulrobertlloyd |
aa6eb5a
to
95ea098
Compare
|
@anandamaryon1 In f1492f1 I’ve spiked support for links appearing in item HTML. This is one to discuss; at its simplest we currently end up with differing focus states; we could make it such that all links in account items are styled such that focus is only applied to the text, not the container. Anyway, here’s my first very quick pass at supporting links in item HTML and the resulting differing focus states: |
@paulrobertlloyd between about 642px and 647px width the header items seem to stack on top of each other, but with left-aligned text, which seems a bit odd? I had a go at stuffing loads of account links in (which hopefully people won't do) - and the account section seems to get a bit too close to the service name. Maybe have a bigger minimum gap? Also not totally sure about the masonry-style layout - you can end up with some odd alignment and some links having a lot more clickable whitespace than others. Maybe this is a reason to switch back to the idea of just having them as plain links with only the text being clickable? I had a quick go here: That gets tricky on mobile though! On another note - from a nunjucks macro point of view, would it be better to just have the 'user' be an 'item', with an extra param to include the icon? Something like: {{ header({
items: [
{
href: "#",
text: "Florence Nightingale",
userIcon: true
},
{
href: "#",
text: "Log out"
}
]
}) }} Then the order would be a bit more flexible? |
7861bb2
to
b4c71cc
Compare
Currently we have a 'log in' version of the header, for when a user is not yet logged in, this seems on the face of it a no-brainer, but I'm wondering whether this is needed given that we have NHS Login and NHS CIS2, which both have their own guidance around logging in, and specific buttons to do so. I'm thinking that the log in link in the header might confuse and not align with the guidance for those two methods. See also our other PR for a log in button: #992 I'm thinking we should remove it. Thoughts? |
@anandamaryon1 our service doesn’t have a login link in the header either but a separate button in the main body of the page (we don’t use CIS2 or NHS login but a separate system). I suspect that having 'login' in the header would only be appropriate for more content based websites (like NHS.UK or possibly others) where you might conceivably want to be able to login from any of the public pages? |
@lasara-d Ooh, good spot! Yeah, that’s not good. Will look if I can come up with a way to achieve the same layout without this side-effect. |
b4c71cc
to
0a13af6
Compare
Ah, it was a 1-line fix; adding |
f5a7da9
to
08add40
Compare
0a13af6
to
2c57068
Compare
This is looking good! A couple of minor points from me: The icon really feels like it ought to be clickable to me? Could we include it within the clickable area if there’s an Both links and non-links in the account area are supported, with examples of both, and they’re differentiated by the presence of the underline. However if we were to drop the underline on all the links in the primary nav (as perhaps it’s unneeded there), would we also then drop the underline on links in the account section? 🤔 |
text: "Check your profile" | ||
}, | ||
{ | ||
html: '<a class="nhsuk-header__account-link" href="#">Notifications</a> <span class="app-count">8</span>' |
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.
Perhaps in this example the notification count out to be part of the link (so that it’s clickable and more accessible if the link is read out out-of-context)? Would save having to add the html for the link tag too.
html: '<a class="nhsuk-header__account-link" href="#">Notifications</a> <span class="app-count">8</span>' | |
href: "#", | |
html: 'Notifications <span class="app-count">8</span>' |
5bfab20
to
8f742dd
Compare
9ab62ce
to
a1ee8c1
Compare
8f742dd
to
39bbc90
Compare
@frankieroberto Updated to include the icon in the href. Will have a look at creating an example with custom HTML for the item and a href. |
Description
Adds the ability to include account information and links in the header.
Resolves nhsuk/nhsuk-service-manual-community-backlog#192
Provides the options for:
account.user.[text|html]
), always shown with a user icon and with the flex layout for this item taking precedence over itemsaccount.items[].[text|html]
), which can be optionally linked (account.items[].href
)These options should provide enough flexibility to use this area in a number of different ways.
Play with these 4 examples (linked from the Examples page) locally to see how layout adjusts in narrower viewports.
To do
nav
)?Single link (example: logged out)
User info and multiple links (example: logged in)
Account, search, navigation and service name (example: the whole enchilada)
Account, search, navigation and organisation name (example: the whole enchilada, also available in white)
Checklist