Refactor greedy navigation #3265

davidrapson · 2024-03-27T17:16:40Z

Summary

Introduces a new version of the greedy navigation component with greatly simplified code and improved behaviour.

Motivation for change

The greedy navigation component is one of our oldest components and was adapted from an external component. It's long been a pain point of the application as it is hard for the team to modify leading to patch work amends and redundant behaviour being maintained.

The main motivation for change is that the component uses the icon font for the toggle button and we'd like to get rid of this in favour of SVG icons for all components.

This requires change. The component is extremely hard to change so we should make the change easy by restructuring the component to only contain the behaviour we need and to match the rest of our components.

The old component also has a completely different API to the rest of our components so this is a good opportunity to make that consistent.

Improvements

Restructuring the component enables a number of improvements:

Switches the icon to an SVG version (the original motivation for change)
Improves keyboard and mouse support (tabbing and clicking in and out of the menu now behaves more consistently)
Minimises flash of unstyled content (fix ported from the public website)
Bundles welsh translations for UI text. Previously this had to be provided as part of the component initialisation (translations taken taken from the public website).
Fixes mistranslation of aria-labels (in production when translated they are part welsh part english)
Removed significant amounts of redundant behaviour that was brought over from the module we modified.

Keyboard support

The old component had partial keyboard support but it was patchy. You could trigger the menu via keyboard but in a non-default way by tabbing in (but only in one direction). The component used a mouse up event instead of a typical click so you couldn't control the button with the space bar like a regular button.

The new behaviour is:

The more button acts as a regular button and can be controlled with the space bar
When the navigation is open you can tab out of it in either direction and the navigation should close (existing behaviour but should work a bit more predictably in both directions)
Clicking anywhere outside the navigation should close the navigation (existing behaviour)
Pressing the escape key should close the navigation (existing behaviour)

Configuration free

The new component is configuration free. The old component provided a number of configuration options many of which were partially implemented or redundant.

The exception being label text which is now translated as part of the Rails template.

The new component is initialised like all our other components.

import { initNavigation } from '@citizensadvice/design-system/lib';
initNavigation();

Testing strategy

Previously we had a mix of tests split across jest and cypress. The jest tests ran assertions against internal methods rather than using testing library helpers. Using jest for component tests is limited as they have to work against static fixtures and don't run in a real browser. Our cypress tests run in a real browser and against real examples but previously only covered some behaviour but not the actual greedy nav logic.

I've taken the approach of keeping a basic jest test around to make sure the component isn't completely broken and then pushed the majority of the test scenarios into the cypress tests. These now cover the logic around moving items into the greedy navigation, keyboard interaction (within the limit of cypress' abilities), and translation behaviour.

Backwards compatibility

To make it easier to migrate to the new component I've kept the old entrypoint around which wraps the new implementation. This allows us to ship the new version and then remove the old entrypoint in a subsequent version.

The exploratory work for this was done in #3264 in case you want to see the process. Depends on #3260

davidrapson · 2024-03-28T12:42:14Z

demo/backstop-common.js

-        label: 'Components/Header',
+        label: 'Components/Header (search open)',


This previously had the same name as another scenario, surprised it didn't clash before!

davidrapson · 2024-03-28T12:43:16Z

demo/app/assets/stylesheets/previews.scss

+.component-preview--navigation,
+.component-preview--header {


Updated these rules so the header component examples also have their margin removed. This is why the header visual regression snapshots have updated for those examples too.

davidrapson · 2024-03-28T12:48:32Z

scss/6-components/_navigation.scss


-/** @define navigation */
+// ============================================================================


I've moved all the navigation styles including single-use mixins into this file as they were previously split across three.

I've also expanded all the selectors so there's minimal shorthand. I've got a strong preference against them and this refactoring reminded me why: it's much nicer if you can just search for the whole classname.

davidrapson · 2024-03-28T12:50:27Z

src/ts/navigation/navigation.test.ts

@@ -0,0 +1,81 @@
+/**


I've kept this around as a basic initialisation test but honestly given the cypress tests cover everything anyway and are much better suited I'm inclined to delete this completely? How do folks feel about that? It's a pretty fake test as it stands.

I think getting rid of it is fine

davidrapson · 2024-03-28T12:52:38Z

src/ts/navigation/navigation.ts

+  return containerEl.clientWidth - padding;
+}
+
+function computeChildrenOffsetWidth(containerEl: HTMLElement) {


This used to have a 10 pixel offset but the rationale in the comment wasn't clear what benefit it gave. We can add it back if others disagree.

Currently we have a couple of papercuts with our packaged javascript modules 1. The method of importing and initialising them is inconsistent 2. The typescript configuration needed is relatively complex for a small number of modules This commit attempts to rationalise that so that we have a consistent pattern for importing and initialising all modules and as a side-benefit a simpler typescript configuration. - Defines a top-level tsconfig.json that works for all modules and removes any per-module configuration needed - Adds consistent index.ts files to every module with a flexible set of default and named exports - Adds a wrapper method to greedy nav to allow it to be initialised in the same way as all other modules The goal is to support the existing import paths whilst enabling a consistent preferred method using named exports: ``` import { initMyModule } from "@citizensadvice/design-system/lib/my-module" ``` I've also included a `lib/index.ts` file so you can import from a single entrypoint: ``` import { initModuleA, initModuleB } from "@citizensadvice/design-system/lib" ``` Ideally we'd support importing directly from `@citizensadvice/design-system` but that needs further restructuring to make this work so something for a future release.

Summary ======= Introduces a new version of the greedy navigation component with greatly simplified code and improved behaviour. Motivation for change ===================== The greedy navigation component is one of our oldest components and was adapted from an external component. It's long been a pain point of the application as it is hard for the team to modify leading to patch work amends and redundant behaviour being maintained. The main motivation for change is that the component uses the icon font for the toggle button and we'd like to get rid of this in favour of SVG icons for all components. This requires change. The component is extremely hard to change so we should make the change easy by restructuring the component to only contain the behaviour we need and to match the rest of our components. The old component also has a completely different API to the rest of our components so this is a good opportunity to make that consistent. Configuration free ================== The new component is configuration free. The old component provided a number of configuration options many of which were partially implemented or redundant. The exception being label text which is now translated as part of the Rails template. The new component is initialised like all our other components. ```js import { initNavigation } from '@citizensadvice/design-system/lib'; initNavigation(); ``` Improvements ============ Restructuring the component enables a number of improvements: 1. Switches the icon to an SVG version (the original motivation for change) 2. Improves keyboard and mouse support (tabbing and clicking in and out of the menu now behaves more consistently) 3. Minimises flash of unstyled content (fix ported from the public website) 3. Bundles welsh translations for UI text. Previously this had to be provided as part of the component initialisation (translations taken taken from the public website). 4. Fixes mistranslation of aria-labels (in production when translated they are part welsh part english) 5. Removed significant amounts of redundant behaviour that was brought over from the module we modified. Backwards compatibility ======================= To make it easier to migrate to the new component I've kept the old entrypoint around which wraps the new implementation. This allows us to ship the new version with everything need as part of the library and we can remove the old entrypoint in a subsequent version.

Causes a conflict with the sticky navigation styles

github-actions · 2024-04-17T10:28:04Z

🚀 Netlify deployed citizens-advice-design-system as draft

https://661fa42e2b512217036c5028--citizens-advice-design-system.netlify.app

davidsauntson

This looks good to me. Some minor comments, it would be good to document doesItFit() a bit more (and maybe call it something more appropriate (it seems to hide / show elements too - but I might have misunderstood!).

davidsauntson · 2024-05-21T13:02:06Z

demo/cypress/e2e/navigation.cy.js

+  // We use https://github.com/dmtrKovalenko/cypress-real-events
+  // to approximate this but full keyboard testing should still be
+  // done manually when changing this behaviour.
+  context('when interacting with a keyboard', () => {


Should we also test tabbing into it? If it's too fraught at least some sort of note to test that behaviour manually too?

I'll need to double check again, I think the answer is still that cypress doesn't handle that well so yes maybe a document that is manual testing steps for greedy nav would be good.

davidsauntson · 2024-05-21T13:03:12Z

design-system-docs/frontend/styles/_base.scss

@@ -7,17 +7,6 @@ body {
  height: 100%;
 }

-// https://philipwalton.github.io/solved-by-flexbox/demos/sticky-footer/


was this causing problems with the navigation?

Yes as a side-effect of porting over the overflow fix from public-website rather than the refactor itself.

davidsauntson · 2024-05-21T13:05:40Z

src/ts/navigation/navigation.test.ts

@@ -0,0 +1,81 @@
+/**


I think getting rid of it is fine

davidsauntson · 2024-05-21T13:08:06Z

src/ts/navigation/navigation.ts

+  return sum;
+}
+
+function doesItFit(containerEl: HTMLElement, breaks: number[]) {


What is breaks here? I think it has something to do with the cumulative widths of each nav link but it would be good to make it explicit (in a comment would be fine I think).

In fact some documentation for this function would be good - it's hard to infer the behaviour from the code fully.

Also it seems to do more than answer the question 'does it fit' - should it be broken up or renamed?

Good shout.

Short answer: Agreed, let's break this down more.

Longer answer: For context the the process I followed was basically to copy over the existing file and names and gradually refactor the implementation, rather than a ground-up rewrite. I'd bet on this being part that retains the most naming of the original.

OK - I'm happy for it to stay like this then and maybe this can be a smaller refactor later down the line. It would be good to have another look at it at some point tho.

davidsauntson · 2024-05-21T13:11:30Z

scss/6-components/_navigation.scss

-    display: none;
+////
+// Prevent overflowing navigation when loaded
+// or when javascript is disable


davidsauntson

Happy for doesItFit to be sorted out in another refactor later 👍🏻

davidrapson force-pushed the greedy-nav-refactor branch 2 times, most recently from b48faf2 to 3959295 Compare March 27, 2024 17:28

davidrapson force-pushed the typescript-cleanup branch 2 times, most recently from 99cc2a2 to 86e43ab Compare March 27, 2024 17:31

davidrapson force-pushed the greedy-nav-refactor branch from 3959295 to 9178f8c Compare March 27, 2024 17:33

davidrapson force-pushed the typescript-cleanup branch from 86e43ab to d23da8c Compare March 27, 2024 17:34

davidrapson force-pushed the greedy-nav-refactor branch from 9178f8c to aebda2b Compare March 27, 2024 17:35

davidrapson force-pushed the typescript-cleanup branch from d23da8c to b0cd126 Compare March 27, 2024 17:38

davidrapson force-pushed the greedy-nav-refactor branch from aebda2b to 44bb450 Compare March 27, 2024 17:39

davidrapson marked this pull request as ready for review March 27, 2024 17:42

davidrapson requested a review from a team as a code owner March 27, 2024 17:42

davidrapson requested review from leeky, akamike, cnorthwood, davidsauntson and marianayovcheva and removed request for a team and leeky March 27, 2024 17:42

davidrapson added the docs preview If this is added a preview version of the docs site will be deployed label Mar 27, 2024

davidrapson force-pushed the greedy-nav-refactor branch 2 times, most recently from a40f707 to eec7980 Compare March 27, 2024 17:47

davidrapson marked this pull request as draft March 28, 2024 09:46

davidrapson force-pushed the greedy-nav-refactor branch from 0a7bd33 to f8c88c8 Compare March 28, 2024 12:38

davidrapson commented Mar 28, 2024

View reviewed changes

davidrapson marked this pull request as ready for review March 28, 2024 12:54

davidrapson force-pushed the typescript-cleanup branch from 3ff4a96 to b9d8377 Compare April 2, 2024 11:35

davidrapson force-pushed the greedy-nav-refactor branch 2 times, most recently from 11cfee1 to 84ed690 Compare April 8, 2024 11:21

davidrapson force-pushed the typescript-cleanup branch from 6b9dc18 to 8ca2a23 Compare April 15, 2024 10:20

davidrapson force-pushed the greedy-nav-refactor branch from 84ed690 to 20eea4d Compare April 15, 2024 10:20

Base automatically changed from typescript-cleanup to main April 17, 2024 09:42

davidrapson added 8 commits April 17, 2024 11:26

Fix hiding of header links

e582f30

Remove margin from header preview

e971e93

Expand cypress test coverage

f3a0fcd

Improve keyboard support

5be132b

Fix header snapshots

54da6d2

Remove flexbox hack from docs theme

6c51428

Causes a conflict with the sticky navigation styles

davidrapson force-pushed the greedy-nav-refactor branch from 20eea4d to 6c51428 Compare April 17, 2024 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor greedy navigation #3265

Refactor greedy navigation #3265

davidrapson commented Mar 27, 2024 •

edited

davidrapson Mar 28, 2024

davidrapson Mar 28, 2024 •

edited

davidrapson Mar 28, 2024 •

edited

davidrapson Mar 28, 2024

davidsauntson May 21, 2024

davidrapson Mar 28, 2024

github-actions bot commented Apr 17, 2024

davidsauntson left a comment

davidsauntson May 21, 2024

davidrapson May 21, 2024

davidsauntson May 21, 2024

davidrapson May 21, 2024

davidsauntson May 21, 2024

davidsauntson May 21, 2024

davidrapson May 21, 2024

davidsauntson May 22, 2024

davidsauntson May 21, 2024

davidsauntson left a comment

		label: 'Components/Header',
		label: 'Components/Header (search open)',


		/** @define navigation */
		// ============================================================================

		.component-preview--navigation,
		.component-preview--header {

Refactor greedy navigation #3265

Are you sure you want to change the base?

Refactor greedy navigation #3265

Conversation

davidrapson commented Mar 27, 2024 • edited

Summary

Motivation for change

Improvements

Keyboard support

Configuration free

Testing strategy

Backwards compatibility

Choose a reason for hiding this comment

davidrapson Mar 28, 2024 • edited

Choose a reason for hiding this comment

davidrapson Mar 28, 2024 • edited

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

github-actions bot commented Apr 17, 2024

davidsauntson 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

davidsauntson left a comment

Choose a reason for hiding this comment

davidrapson commented Mar 27, 2024 •

edited

davidrapson Mar 28, 2024 •

edited

davidrapson Mar 28, 2024 •

edited