From 47e79a91a0a5e7c2d9f67c7867c4eafd65b72b97 Mon Sep 17 00:00:00 2001
From: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Date: Fri, 3 Dec 2021 14:53:08 +0000
Subject: [PATCH] Move navigation classes to the `<nav>`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Move the `govuk-header__navigation` class and any additional classes provided by the `navigationClasses` param to the `<nav>` element rather than the list.

Create a new `govuk-header__navigation-list` for the list, and move the CSS properties that reset the user-agent default list styles and the properties that show and hide the list to this new class.

The `--open` modifier class is now applied to the list (the nav itself now contains the menu button is always visible) so rename it so that it's clear it's applied to the list rather than the nav.

We're making this change because it's more 'correct' and less 'surprising' – users would logically expect the `navigation` BEM element and the `navigationClasses` to be applied to the nav element itself.

There may be a need to allow users to pass classes to apply to the list – but we can do that if and when it comes up. In the meantime, users will still be able to style the list by passing a class `navigationClasses` and targeting the `ul` as a descendant.
---
 src/govuk/components/header/_index.scss      | 16 +++++++++-------
 src/govuk/components/header/header.js        |  4 ++--
 src/govuk/components/header/header.test.js   |  8 ++++----
 src/govuk/components/header/template.njk     |  4 ++--
 src/govuk/components/header/template.test.js |  2 +-
 5 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/src/govuk/components/header/_index.scss b/src/govuk/components/header/_index.scss
index 67ac75469b..9f0c241747 100644
--- a/src/govuk/components/header/_index.scss
+++ b/src/govuk/components/header/_index.scss
@@ -206,16 +206,18 @@
   }
 
   .govuk-header__navigation {
-    display: block;
-    margin: 0;
-    padding: 0;
-    list-style: none;
-
     @include govuk-media-query ($from: desktop) {
       margin-bottom: govuk-spacing(2);
     }
   }
 
+  .govuk-header__navigation-list {
+    // Reset user-agent default list styles
+    margin: 0;
+    padding: 0;
+    list-style: none;
+  }
+
   .js-enabled {
     .govuk-header__menu-button {
       display: block;
@@ -224,14 +226,14 @@
       }
     }
 
-    .govuk-header__navigation {
+    .govuk-header__navigation-list {
       display: none;
       @include govuk-media-query ($from: desktop) {
         display: block;
       }
     }
 
-    .govuk-header__navigation--open {
+    .govuk-header__navigation-list--open {
       display: block;
     }
   }
diff --git a/src/govuk/components/header/header.js b/src/govuk/components/header/header.js
index ca2151941d..cf92b3908f 100644
--- a/src/govuk/components/header/header.js
+++ b/src/govuk/components/header/header.js
@@ -21,7 +21,7 @@ Header.prototype.init = function () {
     return
   }
 
-  this.syncState(this.$menu.classList.contains('govuk-header__navigation--open'))
+  this.syncState(this.$menu.classList.contains('govuk-header__navigation-list--open'))
   this.$menuButton.addEventListener('click', this.handleMenuButtonClick.bind(this))
 }
 
@@ -45,7 +45,7 @@ Header.prototype.syncState = function (isVisible) {
  * sync the accessibility state and menu button state
  */
 Header.prototype.handleMenuButtonClick = function () {
-  var isVisible = this.$menu.classList.toggle('govuk-header__navigation--open')
+  var isVisible = this.$menu.classList.toggle('govuk-header__navigation-list--open')
   this.syncState(isVisible)
 }
 
diff --git a/src/govuk/components/header/header.test.js b/src/govuk/components/header/header.test.js
index 267154bf00..6a546a205f 100644
--- a/src/govuk/components/header/header.test.js
+++ b/src/govuk/components/header/header.test.js
@@ -68,8 +68,8 @@ describe('Header navigation', () => {
       })
 
       it('adds the --open modifier class to the menu, making it visible', async () => {
-        const hasOpenClass = await page.$eval('.govuk-header__navigation',
-          el => el.classList.contains('govuk-header__navigation--open')
+        const hasOpenClass = await page.$eval('.govuk-header__navigation-list',
+          el => el.classList.contains('govuk-header__navigation-list--open')
         )
 
         expect(hasOpenClass).toBeTruthy()
@@ -102,8 +102,8 @@ describe('Header navigation', () => {
       })
 
       it('removes the --open modifier class from the menu, hiding it', async () => {
-        const hasOpenClass = await page.$eval('.govuk-header__navigation',
-          el => el.classList.contains('govuk-header__navigation--open')
+        const hasOpenClass = await page.$eval('.govuk-header__navigation-list',
+          el => el.classList.contains('govuk-header__navigation-list--open')
         )
 
         expect(hasOpenClass).toBeFalsy()
diff --git a/src/govuk/components/header/template.njk b/src/govuk/components/header/template.njk
index 5dcbc8a6d6..96dbed671a 100644
--- a/src/govuk/components/header/template.njk
+++ b/src/govuk/components/header/template.njk
@@ -54,10 +54,10 @@
     </a>
     {% endif %}
     {% if params.navigation %}
-    <nav aria-label="{{ params.navigationLabel | default('Menu') }}">
+    <nav aria-label="{{ params.navigationLabel | default('Menu') }}" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}">
       <button type="button" class="govuk-header__menu-button govuk-js-header-toggle" aria-controls="navigation" aria-label="{{ params.menuButtonLabel | default('Show or hide menu') }}">Menu</button>
 
-      <ul id="navigation" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}" >
+      <ul id="navigation" class="govuk-header__navigation-list">
         {% for item in params.navigation %}
           {% if item.text or item.html %}
             <li class="govuk-header__navigation-item{{ ' govuk-header__navigation-item--active' if item.active }}">
diff --git a/src/govuk/components/header/template.test.js b/src/govuk/components/header/template.test.js
index 4a5b185339..c0b884140f 100644
--- a/src/govuk/components/header/template.test.js
+++ b/src/govuk/components/header/template.test.js
@@ -109,7 +109,7 @@ describe('header', () => {
       const $ = render('header', examples['with navigation'])
 
       const $component = $('.govuk-header')
-      const $list = $component.find('ul.govuk-header__navigation')
+      const $list = $component.find('ul.govuk-header__navigation-list')
       const $items = $list.find('li.govuk-header__navigation-item')
       const $firstItem = $items.find('a.govuk-header__link:first-child')
       expect($items.length).toEqual(4)