Implement accordion for mobile navigation #4701

romaricpascal · 2025-05-16T17:17:57Z

Ports the code of #4368, splitting implementation into individual commits for the template, JavaScript and CSS. As a high level overview, the PR enhances the Service Navigation to provide the same accordion behaviour we have on the current navigation on mobile.

It does so by rendering inside each of the Service Navigation link a <template> tag with the markup of the subnavigation for that section of the site. That template is then picked up by a new MobileNavigationSection JavaScript component, instantiated for each link.

The component injects the content of the template on the page, as well as a <button> that'll let users toggle the visibility of the subnavigatio. It is also responsible for hiding the original link from the Service Navigation on mobile, and hiding the button and subnavigation on wider viewports. This allows the CSS to not rely on media queries for its styling.

In more details this PR:

introduces a new _mobile-navigation.njk partial responsible for rendering the subnavigation for each item of the Service Navigation
uses that new partial in the _navigation.njk partial where the Service Navigation is rendered
introduces a new MobileNavigationSection JavaScript component for enhancing the markup, toggling the subnavigation and controling its visibility based on the viewport
adds the necessary styles to display the accordion, including chevrons to highlight that sections can be expanded inside the navigation

netlify · 2025-05-16T17:19:05Z

✅ You can preview this change here:

Name	Link
🔨 Latest commit	`9dbcff5`
🔍 Latest deploy log	https://app.netlify.com/projects/govuk-design-system-preview/deploys/683985ce9e51020008e2bc1e
😎 Deploy Preview	https://deploy-preview-4701--govuk-design-system-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

romaricpascal · 2025-05-20T18:41:15Z

Opened a Merge request on html-validate's repo to fix the rule that's mistaken so hopefully we'll be unblocked soon 🥳

In the meantime, I'd welcome a first look from anyone in @alphagov/design-system-developers on the technical side of things. There might be small tweaks for the design, but would be good to know if everything looks OK 😊

src/javascripts/components/mobile-navigation-section.mjs

romaricpascal · 2025-05-20T18:54:39Z

src/javascripts/components/mobile-navigation-section.mjs

+  createButton() {
+    const $button = document.createElement('button')
+    $button.classList.add('govuk-service-navigation__link')
+    $button.classList.add('app-mobile-navigation-section__button')
+    // Ensure no whitespace coming from the server rendered Service Navigation
+    // link gets into the button as it would mess the underline on hover
+    $button.textContent = this.$root.textContent.trim()
+    $button.setAttribute('aria-expanded', 'false')
+    $button.hidden = true
+
+    return $button
+  }


question Wondering if this would be better implemented as a getter that caches its result, doing the same for the subnav? Logic behind it would be to avoid the createXYZ methods, but that might not be that much of a gain 🤔

get $button() { if (!this._$button) { const $button = document.createElement('button') $button.classList.add('govuk-service-navigation__link') $button.classList.add('app-mobile-navigation-section__button') // Ensure no whitespace coming from the server rendered Service Navigation // link gets into the button as it would mess the underline on hover $button.textContent = this.$root.textContent.trim() $button.setAttribute('aria-expanded', 'false') $button.hidden = true this._$button = $button; } return this._button; }

mia-allers-gds

This looks great! Only thing I've noticed is this double border in forced colour mode happening because each button section has bottom padding on it. Only the last button needs extra padding on the bottom.

Forced colour mode showing a double border on each button

Default colour mode showing extra padding at the bottom of the buttons

hazalarpalikli · 2025-05-28T11:20:02Z

Looks good to me

owenatgov

A few little comments. Looking solid otherwise!

owenatgov · 2025-05-28T14:12:59Z

src/stylesheets/components/_mobile-navigation-section.scss

+  font: inherit;
+  text-align: left;
+
+  // Add some extra affordance for mouse users


Extremely pithy point: is this comment necessary? I'm not sure what else this could be for.

owenatgov · 2025-05-28T15:01:49Z

views/partials/_mobile-navigation.njk

@@ -0,0 +1,43 @@
+{% macro mobileNavigationItem(params) %}
+  {% set current = true if params.pagePermalink and (params.pagePermalink == params.item.url) else false %}
+  {% set active = params.active if 'active' in params else (true if params.pagePermalink and params.pagePermalink.startsWith(params.item.url) else false) %}


How come this can't be if params.active?

Yeah, that needs some explanation... and maybe a little rework on my part. params.active is there to override the default computed in the macro so that the first item of the subnav (eg. 'Components overview') is never 'active' as it would visually show the 'active' state when navigating a page of the subnav (eg. 'Button').

I'll rename the parameter to something more suitable (eg. ignoreActive) and leave a little comment as well.

36degrees

I'm a little bit nervous about just how much we're overriding from the service navigation component on mobile – I'm concerned that we make changes to the component in GOV.UK Frontend that break the custom implementation here and don't notice. I don't think we really pay much attention to what the site looks like on mobile when e.g. updating GOV.UK Frontend.

I'm not sure what the solution to that is though – we could for example fork the component, but that does somewhat defeat the point of the exercise!

Maybe we should be considering introducing visual regression tests to the site at some point in the future 🤔

A couple of other things that I've noticed:

the co-authored by trailer on 60d0508 doesn't look right
a lot of comments seem to be wrapped semi-randomly, though I think I might be the only person who's bothered by this!

36degrees · 2025-05-28T12:53:22Z

__tests__/mobile-navigation-section.test.js

+  })
+
+  describe('when JavaScript is unavailable or fails', () => {
+    beforeEach(async () => {


Do we need to repeat this for each test or is it enough to use beforeAll and do it once at the start of the describe block?

Will need to investigate when Jest tears down pages between tests, but if it doesn't we could use beforeAll.

36degrees · 2025-05-28T12:53:32Z

__tests__/mobile-navigation-section.test.js

+  })
+
+  describe('when JavaScript is available', () => {
+    beforeEach(async () => {


Do we need to repeat this for each test or is it enough to use beforeAll and do it once at the start of the describe block?

We probably do need this to be beforeEach because some of the nested describe blocks take us to other pages.

But the way the tests are structured makes this hard to follow – it's not particuarly clear for example which page the tests in the User interactions describe block are running on.

I'll have a think at how to clarify that. Could always move the goTo at the start of the tests rather than in beforeEach/All to make it clearer what they run on 😊

36degrees · 2025-05-28T12:54:19Z

__tests__/mobile-navigation-section.test.js

+      })
+
+      describe('On a section root', () => {
+        beforeEach(async () => {


Do we need to repeat this for each test or is it enough to use beforeAll and do it once at the start of the describe block?

36degrees · 2025-05-28T12:54:24Z

__tests__/mobile-navigation-section.test.js

+      })
+
+      describe('In a page within a section', () => {
+        beforeEach(async () => {


Do we need to repeat this for each test or is it enough to use beforeAll and do it once at the start of the describe block?

romaricpascal · 2025-05-29T09:10:30Z

I'm a little bit nervous about just how much we're overriding from the service navigation component on mobile – I'm concerned that we make changes to the component in GOV.UK Frontend that break the custom implementation here and don't notice. I don't think we really pay much attention to what the site looks like on mobile when e.g. updating GOV.UK Frontend.

I'm not sure what the solution to that is though – we could for example fork the component, but that does somewhat defeat the point of the exercise!

Maybe we should be considering introducing visual regression tests to the site at some point in the future 🤔

That's a fair point. Visual regression tests sound a good shout (opening a components page and opening the menu would allow us to check that the subnav renders OK, including the active section and item).

A couple of other things that I've noticed:

* the co-authored by trailer on [60d0508](https://github.com/alphagov/govuk-design-system/commit/60d0508bf42221a43b8621e3bcc2c046c3ff2d5e) doesn't look right

Whoops, muscle-memory took over 😓 Thanks for spotting it!

* a lot of comments seem to be wrapped semi-randomly, though I think I might be the only person who's bothered by this!

I'll have a quick look as I edit things :)

romaricpascal · 2025-05-29T17:21:02Z

I think I've gone through all the comments, hopefully:

how the active state of sub-navigation items is computed is clearer, particularly for the first item
the order of tests makes it easier to understand which page they run on. I've moved the tests that run on different pages to the end of the tests. @36degrees I've gone the other direction with beforeAll/beforeEach and made the existing beforeAll into a beforeEach. Reason for it is to properly control the environment each test runs in as some tests change the page that's loaded and some change the viewport, making it a risk they break subsequent tests if they fail and we don't reset both the viewport size and URL.
the comments are wrapped in a more sensible way

romaricpascal · 2025-05-30T09:20:57Z

Force pushed one last time to fix the formatting that Prettier wasn't happy with. Once #4710 is ready to go, this one and #4668 can be merged

Ports the code of #4368 for the rendering of the subnavigation of each item of the Service Nav in a `<template>` element which will then be used by JavaScript. Co-Authored-By: [email protected]

Instantiate one component per section of the navigation to easily group the toggle button and subnavigation it reveals Co-Authored-By: [email protected]

As the mobile navigation is only available when JavaScript is loaded, and gets shown/hidden through the `hidden` HTML attribute, styles do not need to be gated by a media query. As the links live withing the Service Navigation, we can reuse its `govuk-service-navigation__link` styles for the interactive elements (subnavigation toggle and links) Co-Authored-By: [email protected]

Include the fix that makes `<template>` tags correctly handled by `element-permitted-content`. https://gitlab.com/html-validate/html-validate/-/issues/305

romaricpascal force-pushed the service-nav-accordion branch 4 times, most recently from 79dbca4 to 35b5b02 Compare May 20, 2025 15:58

romaricpascal changed the title ~~[WIP] Implement accordion for mobile navigation~~ Implement accordion for mobile navigation May 20, 2025

romaricpascal requested review from hazalarpalikli and mia-allers-gds May 20, 2025 17:03

romaricpascal commented May 20, 2025

View reviewed changes

src/javascripts/components/mobile-navigation-section.mjs Show resolved Hide resolved

romaricpascal linked an issue May 20, 2025 that may be closed by this pull request

Implement accordion into service navigation #4700

Closed

4 tasks

romaricpascal force-pushed the service-nav-accordion branch from 35b5b02 to 7e96525 Compare May 20, 2025 18:54

romaricpascal commented May 20, 2025

View reviewed changes

romaricpascal force-pushed the service-nav-accordion branch from 7e96525 to 9020aab Compare May 23, 2025 18:12

romaricpascal changed the base branch from spike-flat-mobile-nav to spike-breadcrumbs-all-pages May 23, 2025 18:15

romaricpascal force-pushed the service-nav-accordion branch from 9020aab to 91d1a61 Compare May 23, 2025 18:18

romaricpascal marked this pull request as ready for review May 27, 2025 11:57

romaricpascal mentioned this pull request May 27, 2025

'Flat' implementation of the service navigation component #4668

Merged

mia-allers-gds reviewed May 27, 2025

View reviewed changes

romaricpascal force-pushed the spike-breadcrumbs-all-pages branch from bf52b3a to 535e8ea Compare May 27, 2025 14:02

romaricpascal force-pushed the service-nav-accordion branch 2 times, most recently from 24ceac1 to 3a99b89 Compare May 27, 2025 17:16

hazalarpalikli approved these changes May 28, 2025

View reviewed changes

owenatgov reviewed May 28, 2025

View reviewed changes

36degrees self-requested a review May 29, 2025 07:50

36degrees reviewed May 29, 2025

View reviewed changes

romaricpascal force-pushed the service-nav-accordion branch 2 times, most recently from aab0862 to 4d0916a Compare May 29, 2025 17:18

romaricpascal requested a review from 36degrees May 29, 2025 17:21

36degrees approved these changes May 30, 2025

View reviewed changes

romaricpascal force-pushed the service-nav-accordion branch from 4d0916a to 122eca3 Compare May 30, 2025 09:16

romaricpascal added 4 commits May 30, 2025 11:17

Render subnavigation in a <template> for mobile

05b4487

Ports the code of #4368 for the rendering of the subnavigation of each item of the Service Nav in a `<template>` element which will then be used by JavaScript. Co-Authored-By: [email protected]

Add JavaScript behaviour to accordion sections in the mobile menu

143129b

Instantiate one component per section of the navigation to easily group the toggle button and subnavigation it reveals Co-Authored-By: [email protected]

Update html-validate

9dbcff5

Include the fix that makes `<template>` tags correctly handled by `element-permitted-content`. https://gitlab.com/html-validate/html-validate/-/issues/305

romaricpascal force-pushed the service-nav-accordion branch from 122eca3 to 9dbcff5 Compare May 30, 2025 10:17

romaricpascal merged commit 58b2e8b into spike-breadcrumbs-all-pages May 30, 2025
13 checks passed

romaricpascal deleted the service-nav-accordion branch May 30, 2025 10:47

36degrees mentioned this pull request May 30, 2025

Review the use of aria-label on a <ul> element in the site navigation menu #2624

Open

2 tasks

romaricpascal mentioned this pull request May 30, 2025

Import ServiceNavigation, implement MobileNavigation, remove Navigation #4368

Closed

Implement accordion for mobile navigation #4701

Implement accordion for mobile navigation #4701

Uh oh!

Conversation

romaricpascal commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ You can preview this change here:

Uh oh!

romaricpascal commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

romaricpascal May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mia-allers-gds left a comment

Choose a reason for hiding this comment

Uh oh!

hazalarpalikli commented May 28, 2025

Uh oh!

owenatgov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romaricpascal May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

36degrees left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romaricpascal commented May 29, 2025

Uh oh!

romaricpascal commented May 29, 2025

Uh oh!

romaricpascal commented May 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

romaricpascal commented May 16, 2025 •

edited

Loading

netlify bot commented May 16, 2025 •

edited

Loading

romaricpascal commented May 20, 2025 •

edited

Loading

romaricpascal May 20, 2025 •

edited

Loading

romaricpascal May 29, 2025 •

edited

Loading