Skip to content
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

[WIP] Service Header component #4950

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Apr 22, 2024

Takes the 'Service Header' concept from #4915 and implements it as a standalone component, with the toolbox/dropdown concept removed (at least for now).

For alphagov/govuk-design-system#3748.

Links

Todo

Creating a Service Header component

  • There are a few minor design issues outstanding:
    • The height of the Service Header (on desktop) is different depending on whether there are navigation links present or just a service name.
    • The focus style is positioned differently depending on whether it's a service name link or a navigational link.
    • The mobile layout generally feels a little bit unevenly spaced and could use some tweaking.
  • The injection of <strong> for active links is done a bit haphazardly and needs more consideration as to when it's used and how it's styled.
  • Comprehensive review app examples and associated tests written.
    • Template tests.
    • JS functionality tests.
    • Review app examples added/updated.
  • Readme content written (this can probably be mostly lifted from another component).

Changes to other components

  • Move the <header> tag out of the Header component and into the template.
  • Adapt the Phase banner to be usable between the Header and Service Header components.
  • Add 'slots' to the Header and Service Header components.
  • Deprecate the navigation and service name/URL options on the Header component.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@Ciandelle
Copy link

Overall I'm not seeing any glaring issues that concern me. I do wonder if the blue text is something to change based on the working group feedback. Additionally, personally I don't like the layout of the service header with large navigation, it feels too unorganised and I would struggle to find what I'm looking for. I am aware that this may be a complete personal thing, so please don't get caught up on it

@querkmachine
Copy link
Member Author

personally I don't like the layout of the service header with large navigation, it feels too unorganised and I would struggle to find what I'm looking for.

That's entirely fair. This is one of those review app examples which is intended to test the extremes of the component. No one should actually be using that many navigation items in practice, I hope! 🤞

@CharlotteDowns CharlotteDowns marked this pull request as ready for review May 7, 2024 08:22
@CharlotteDowns CharlotteDowns marked this pull request as draft May 7, 2024 08:23
@selfthinker
Copy link

selfthinker commented May 7, 2024

I've done some accessibility testing of the worst scenario example page. Here are the results.

Navigation landmark missing in mobile view

The menu button (which shows on the mobile view or when zoomed in) was previously inside the nav element and was now moved out. A consequence of that is that the navigation landmark disappears completely on mobile (or when zoomed in) and is not discoverable when the navigation is collapsed. This happened in all the screen readers I tested with.

Because I had mentioned this elsewhere before, @querkmachine has fixed it today. I just wanted to mention it for completeness' sake.

Also, not a problem, just something I noticed is that the navigation now collapses at a narrower breakpoint than before. Mentioning it just in case it was not intended.

Unlabelled breadcrumbs confusing

The navigation coming right before the breadcrumbs exacerbate the issue that the breadcrumbs don't have a label. It is very unclear what those links are when you cannot see their design visually. A screen reader will say that they are out of the navigation landmark, but they appear to belong to the navigation.

This is not something new and most probably out of scope. Just mentioning it because it's easier to notice when testing both navigation and breadcrumbs together.

Position of phase banner makes service name less clear

The phase banner being between the logo and the service name makes it harder to understand what the service name is. It previously would have read (simplified): "GOV.UK - Manage company information - Alpha This is a new service – your feedback will help us to improve it." Now it reads (simplified): "GOV.UK - Alpha This is a new service – your feedback will help us to improve it. - Manage company information". The service name seems "lost" at a later point.
I have made a few screenshots with CSS disabled (and the landmarks showing) to hopefully make the issue more visible.

The previous version with the phase banner after the navigation:
Screenshot showing first logo, then service name, then service navigation, then phase banner

The current version with the phase banner before the service name:
Screenshot showing first logo, then phase banner, then service name, then service navigation

Navigation difficult to discover when magnified

The navigation is currently right-aligned when it comes after the service name (in desktop view). That will make it harder for magnification users to discover. The horizontal lines help a bit. But I'd suggest thinking about left-aligning them instead.

All items highlighted when changing colours

When changing colours via High Contrast Mode or Firefox browser settings all the items will appear highlighted, not just the active one. That is both the case for the desktop as well as mobile (or zoomed in) view.

Screenshot showing behaviour in desktop view described above
Screenshot showing behaviour in mobile view described above

Menu chevron missing in High Contrast Mode

When changing colours via High Contrast Mode the menu chevron is missing, which removes the visual affordance of understanding that it is clickable and will expand something.

Screenshot showing behaviour described above

NVDA in Chrome behaving weird

When using NVDA in Chrome it behaves a bit weird. It doesn't read the service name when arrowing. (It does read it when tabbing.) It also reads all the navigation items in one go, meaning I cannot navigate them individually via arrowing. (Again, it reads them when tabbing.) That is only the case for the desktop view, not for the mobile (or zoomed in) view.

I wonder if this has something to do with flexbox? It's possible it's a bug in NVDA (or Chrome?) rather than something wrong in our code.
NVDA is working as expected in Firefox, and JAWS is working as expected in Chrome in this regard.

@querkmachine
Copy link
Member Author

Position of phase banner makes service name less clear

This is a very good point, and is useful evidence against calls to let the phase banner live in this location.

I imagine the situation would be different if we let the service name still live in the header, rather than moving it to this new component.

Menu chevron missing in High Contrast Mode

Just a quick check if this was tested in Edge or Chrome? If so, this is likely because of a High Contrast Mode bug introduced in the latest versions of those browsers. alphagov/reported-bugs#87

Happily, this looks like it should be fixed for the next releases of those browsers.

All items highlighted when changing colours

This is, in hindsight, not unexpected. Whoops! Definitely something that needs fixing.

@selfthinker
Copy link

Position of phase banner makes service name less clear

I imagine the situation would be different if we let the service name still live in the header, rather than moving it to this new component.

Yes, when the service name is in the global header, the phase banner could (or maybe even should?) come right after.

By the way, it's not just the phase banner that can make the service name less clear, the One Login navigation and a search bar will have the same effect. I wonder if we should think about those cases as well as the experience of those will be worse after the service name has moved to the service banner.

Menu chevron missing in High Contrast Mode

Just a quick check if this was tested in Edge or Chrome? If so, this is likely because of a High Contrast Mode bug introduced in the latest versions of those browsers. alphagov/reported-bugs#87

Yes, this was in Edge.
I saw you posting about that bug but didn't think about it when I found this. That looks like the culprit indeed.

@querkmachine
Copy link
Member Author

I've pushed up a fix for all of the links appearing selected in High Contrast Mode.

The other things I haven't actioned yet could affect the overall design of the component and probably needs to do the rounds within the squad first

Update existing full-page examples that have a service name or navigation to use the Service Header component instead.

Also update header and phase banner styles to work with service header:

- Makes header border full-width in all circumstances
- Adds content width container to phase banner
- Change a Notification Banner test that created a second banner role, as this created an accessibility failure with the header always being present in the template
Remove the 'with no options set' example from being tested as part of the standard accessibility tests.

This is because the accessibility test tries to initialise the component's JS, but there is no HTML being rendered to initialise it against, returning an ElementError that Jest considers to be a test failure.
@selfthinker noted that having the entire `<nav>` element toggled means that the navigation landmark is inaccessible when the menu is closed. Not having the toggle within the landmark also means that assistive technology users wouldn't be able to jump to it easily.

This commit moves the toggle button to within the `<nav>` element and tweaks the JS so that the `<ul>` has its visibility toggled instead.
The method by which component JS is loaded and initialised changed in Frontend 5.4.0. This commit updates some of the references that didn't get caught while rebasing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress 📝
Development

Successfully merging this pull request may close these issues.

Get something working in Frontend
5 participants