-
Notifications
You must be signed in to change notification settings - Fork 109
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
Header updates - breaking changes #1058
base: main
Are you sure you want to change the base?
Conversation
@paulrobertlloyd found an issue with your logo SVG change (#1047), when printing the NHS logo no longer shows. Not looked into the code but I assume it's just missing a rule to change it's colour from white to blue on print? |
c2ccf58
to
fa05a47
Compare
f5a7da9
to
08add40
Compare
Removes the link to `"/"` labelled `"Home"`, which is currently hardcoded, and only shows up within the navigation menu on mobile screen widths. This link may not be appropriate for all services, which might not have a homepage, or might use a different path for it. It is also unclear whether having a homepage link is always needed in the navigation if the NHS logo also goes to the homepage.
9ab62ce
to
a1ee8c1
Compare
@paulrobertlloyd could we avoid force pushing to this branch for a bit? Makes it harder to maintain the other pull requests into this branch. We can sort out the commit history at the end? |
* Update header navigation label --------- Co-authored-by: Frankie Roberto <[email protected]> Co-authored-by: anandamaryon1 <[email protected]>
* Add account links to header * Don’t link to header example with custom HTML * Update backstop references for header component after adding account information to this component * Remove unused backstop images for header component * Add backstop images for header component with account links
Accessibility and device testingTested with NVDA, JAWS and VoiceOver, all work as expected. Tested on Safari on Mac, and Edge and Chrome Windows. Android devices Galaxy 21 Samsung browser and Pixel 6 Chrome, all as expected. Tested on iOS Safari, all as expected on version 15 and above. IssuesTwo issues discovered on Safari 14 (iPhone 8, iPad mini 2019 & iPad air 4) and below:
These issues do not affect the current header implementation. Raises a wider question of our browser support which has not been updated in 3 years. nhsuk/nhsuk-service-manual#2063 |
These are used by by the `header-account-custom-html` test, and I can't see a way to turn off the rule for just that one file...
Following on from a previous comment on underlining the nav links, I noticed gov guidance on underlining links...
I mocked up a header for the NHS App (using the new 'account info' variant)... ![]() I thought I'd see how it looked without underlines (and removed the ![]() |
@davidhunter08 Horizontal justification of the links also helps in your example. It works well on nhs.uk due to the number and length of links, but as a rule for all headers it may not make so much sense. If we're going to do it, this is the time and this is the PR. Thoughts @frankieroberto @paulrobertlloyd ? |
|
➕1 for removing the underlines from the horizontal nav (except maybe on hover) and removing the default justification. It would also fix the currently-misleading issue that the 'More' toggle button uses an underline, and so might be mistaken for a link. |
I've resolved the conflicts and fixed any new Stylelint issues Presume the visual regression tests failed before? |
Issues found during accessibility and device testing@anandamaryon1 I looked at how we might use margins instead for navigation items instead of flexbox in #1138, but it got complicated quickly (given we’re also supporting optional justification, and JS-enabled items moving into a collapsible menu). I suspect the same is true for other areas where the design falls apart a little due to its reliance on using I think the bunched up navigation items are the primary concern. This was previously mitigated against because items had generous padding to their left and right. With that padding removed, navigation items will line up closer to each other. Safari on iOS is a pain because users can’t upgrade Safari, and any other browser on the platform is forced to use the same underlying Webkit rendering engine. Once OS updates are no longer provided, the user is stuck with the last supported version of WebKit, which in the above case is 14. This is even more so annoying because this version supports grid layout and therefore Curious to hear if others have suggestions for how/if we can fix the above noted issues for older browsers. @frankieroberto @colinrotherham |
Following on from the above, #1138 (which left-aligns navigation items by default) should now also fix the issue of navigation items appearing too close together in Safari 14. |
DSRP account header feedback summaryThe design system review panel has reviewed this component and provided the following feedback, which we will look to address before releasing. Component design (frontend)
Component guidance (service manual)
|
Description
Bringing together a series of breaking changes from the following PRs:
Checklist