Add Service Navigation component, add new Mobile Navigation component #4368

patrickpatrickpatrick · 2024-12-06T12:07:45Z

What

Why

netlify · 2024-12-06T12:09:41Z

✅ You can preview this change here:

Name	Link
🔨 Latest commit	`67302af`
🔍 Latest deploy log	https://app.netlify.com/sites/govuk-design-system-preview/deploys/6764447d29f2ea0008b4a743
😎 Deploy Preview	https://deploy-preview-4368--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 site configuration.

romaricpascal

Listed the comments we had during our call. Once the accordion is in place, there are these two other responsibilities that the current component handles and that we'll need to implement in the new one:

Starting with the subnav open on the current page if not on the homepage
Handling if CSS fails to load, but JavaScript loads by using hidden attributes
- Responding to screen-size changes by setting hidden attributes as well

romaricpascal · 2024-12-16T15:33:01Z

src/javascripts/application.mjs

+      const newButton = document.createElement('button')
+      newButton.classList.add('app-navigation__list-toggle')
+      newButton.setAttribute('subitems-toggle-closed', true)
+      newButton.innerHTML = '▼'


suggestion Use the textContent from the link to create the button name and hide the link on mobile.

romaricpascal · 2024-12-16T15:34:06Z

src/javascripts/application.mjs

+const serviceNavigationHeader = document.querySelector(
+  '.govuk-service-navigation'
+)


suggestion Create a MobileNavigation component set up on a <div> that wraps the service navigation.

romaricpascal · 2024-12-16T15:34:47Z

views/partials/_sub-navigation-items.njk

+{% macro subNavigationItems(params) %}
+  {% if params.items %}
+      <ul class="app-navigation__list js-app-navigation__list--hidden">
+        {% for item in params.items %}


issue Accordions in the current navigation have a first item linking to the section, replacing the link that's getting hidden.

romaricpascal · 2024-12-16T15:35:13Z

views/partials/_navigation.njk

+    {% set subNavItemHtml = subNavigationItems({ items: item.items }) %}
+    {% set navItem = {
+      href: item.href,
+      html: item.text + '<template>' + subNavItemHtml + '</template>',


suggestion Setting a class on the <template> will help access it from the JavaScript.

romaricpascal · 2024-12-16T15:35:52Z

src/javascripts/application.mjs

+if (
+  serviceNavigationHeader.hasAttribute('data-govuk-service-navigation-init')
+) {
+  const subNavTreeParents = serviceNavigationHeader.querySelectorAll('li')


suggestion We can access the links and use insertAdjacentElement to inject the sub navigation and button.

romaricpascal · 2024-12-16T15:38:21Z

src/javascripts/application.mjs

+        if (e.target.hasAttribute('subitems-toggle-closed')) {
+          subNav.classList.remove('js-app-navigation__list--hidden')
+          e.target.removeAttribute('subitems-toggle-closed')


suggestion Instead of having our own attribute, we can use the aria-expanded attribute which will be necessary for accessibility.

romaricpascal · 2024-12-16T15:39:27Z

src/javascripts/application.mjs

+        const subNav = subNavParent.querySelector('.app-navigation__list')
+
+        if (e.target.hasAttribute('subitems-toggle-closed')) {
+          subNav.classList.remove('js-app-navigation__list--hidden')


suggestion We could use the next sibling selector to toggle the visibility of the subnav based on the state of aria-expanded. https://www.htmhell.dev/adventcalendar/2024/16/

romaricpascal

Nice to see the feature gathered in its own component and generally going in the right direction. Thanks for updating 🙌🏻

I've dropped comments where I've noticed things with the current implementation (some of them which may become addressed by the replication of the feature that handles JS loading but CSS not loading).

romaricpascal · 2024-12-17T17:35:26Z

src/javascripts/application.mjs

 // Initialise mobile navigation
 createAll(Navigation)


suggestion Given we're using a new component we can drop this bit :)

Suggested change

// Initialise mobile navigation

createAll(Navigation)

romaricpascal · 2024-12-17T17:38:12Z

src/javascripts/components/mobile-navigation.mjs

+    this.templates = this.$root.querySelectorAll(
+      '.app-mobile-navigation__template'
+    )
+    this.links = this.$root.querySelectorAll('a')


issue This has a risk to collect elements more broadly than we intend, if one day we add some content to the navigation through slots, for example. Would be good to limit this to links from the actual navigation using their class from the Service Navigation (unless we can set up our own .js- prefixed class).

romaricpascal · 2024-12-17T17:42:09Z

src/javascripts/components/mobile-navigation.mjs

+
+    // A global const for storing a matchMedia instance which we'll use to detect when a screen size change happens
+    // Set the matchMedia to the govuk-frontend tablet breakpoint
+    this.mql = window.matchMedia('(min-width: 40.0625em)')


suggestion We can use the CSS custom properties set by GOV.UK Frontend to get the breakpoint value.

romaricpascal · 2024-12-17T17:43:13Z

src/javascripts/components/mobile-navigation.mjs

+      },
+      { bubbles: true }


issue bubbles is not one of the options of addEventListener

Suggested change

},

{ bubbles: true }

}

romaricpascal · 2024-12-17T17:49:06Z

src/javascripts/components/mobile-navigation.mjs

+ * Mobile Navigation enhancement for Service Navigation component
+ */
+class MobileNavigation extends Component {
+  static moduleName = 'mobile-navigation'


issue For consistency with the other components, let's make sure to prefix the module name with app-

Suggested change

static moduleName = 'mobile-navigation'

static moduleName = 'app-mobile-navigation'

romaricpascal · 2024-12-17T17:51:27Z

src/stylesheets/components/_mobile-navigation.scss

+  border-top: govuk-px-to-rem(2px) solid;
+  border-right: govuk-px-to-rem(2px) solid;


Solid way to have the chevron visible for people using high contrast mode 🙌🏻

romaricpascal · 2024-12-17T17:57:44Z

src/stylesheets/components/_mobile-navigation.scss

+.app-mobile-navigation__toggle-button[aria-expanded="true"] ~ .app-navigation__list {
+  @include govuk-media-query($until: tablet) {
+    display: block;
+  }
+
+  display: none;
+}


suggestion Hiding the navigation list and toggle would gain at being regrouped in a single block of code and a single media query for easier maintainability rather than split in their own selector, in the lines of:

Suggested change

.app-mobile-navigation__toggle-button[aria-expanded="true"] ~ .app-navigation__list {

@include govuk-media-query($until: tablet) {

display: block;

}

display: none;

}

@include govuk-media-query($from: tablet) {

.app-mobile-navigation__toggle-button,

.app-navigation__list {

display: none;

}

}

This would allow the file to describe first how the different parts look, then when they're visible or not. That media query may also not be necessary once the JavaScript handles using hidden to ensure elements are not visible if CSS did not load.

romaricpascal · 2024-12-17T17:58:20Z

src/stylesheets/components/_mobile-navigation.scss

+.app-mobile-navigation__toggle-button[aria-expanded="true"]::after {
+  transform: rotate(-45deg);
+}


suggestion This can likely be the default state of the button and save a selector.

romaricpascal · 2024-12-17T18:09:07Z

src/stylesheets/components/_mobile-navigation.scss

+.app-mobile-navigation__toggle-button[aria-expanded="false"] ~ .app-navigation__list {
+  display: none;
+}


We may be able to let that selector go when we re-implement the current navigation feature for using hidden to hide elements when CSS fails to load.

romaricpascal · 2024-12-17T18:19:53Z

src/stylesheets/main.scss

+.app-width-container,
+.govuk-service-navigation .govuk-width-container {


This may be an indication that we may want to update the CSS variable in this stylesheet and add a custom width container to the examples 🤔 This is something we can explore in a future PR, though 😊

CharlotteDowns and others added 3 commits December 5, 2024 13:33

add example service nav component

a74403b

get current section working

be78ae2

link html

8e7365a

patrickpatrickpatrick added 2 commits December 9, 2024 16:41

diff approach

142d49e

partials for accoridon within nav

97c632f

romaricpascal reviewed Dec 16, 2024

View reviewed changes

patrickpatrickpatrick force-pushed the add-service-nav-website branch from d6b7fb6 to 9007223 Compare December 17, 2024 16:16

romaricpascal reviewed Dec 17, 2024

View reviewed changes

patrickpatrickpatrick changed the title ~~Add service nav website~~ Add Service Navigation component, add new Mobile Navigation component Dec 18, 2024

js functionality of accoridon within nav

d18d80c

patrickpatrickpatrick force-pushed the add-service-nav-website branch from 9007223 to d18d80c Compare December 18, 2024 11:07

patrickpatrickpatrick added 3 commits December 18, 2024 11:16

update config of navigation

a7fb2d3

add overview links, active style in subnav

70a62a4

add back themes

67302af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Service Navigation component, add new Mobile Navigation component #4368

Add Service Navigation component, add new Mobile Navigation component #4368

patrickpatrickpatrick commented Dec 6, 2024 •

edited

Loading

netlify bot commented Dec 6, 2024 •

edited

Loading

romaricpascal left a comment

romaricpascal Dec 16, 2024

romaricpascal Dec 16, 2024

romaricpascal Dec 16, 2024

romaricpascal Dec 16, 2024

romaricpascal Dec 16, 2024

romaricpascal Dec 16, 2024

romaricpascal Dec 16, 2024

romaricpascal left a comment

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

romaricpascal Dec 17, 2024

	static moduleName = 'mobile-navigation'
	static moduleName = 'app-mobile-navigation'

		border-top: govuk-px-to-rem(2px) solid;
		border-right: govuk-px-to-rem(2px) solid;

		.app-width-container,
		.govuk-service-navigation .govuk-width-container {

Add Service Navigation component, add new Mobile Navigation component #4368

Are you sure you want to change the base?

Add Service Navigation component, add new Mobile Navigation component #4368

Conversation

patrickpatrickpatrick commented Dec 6, 2024 • edited Loading

What

Why

netlify bot commented Dec 6, 2024 • edited Loading

✅ You can preview this change here:

romaricpascal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickpatrickpatrick commented Dec 6, 2024 •

edited

Loading

netlify bot commented Dec 6, 2024 •

edited

Loading