Refactor JavaScript for tabs functionality

[querkmachine]

Refactors the JavaScript associated with the tabbed interface used for code examples.

Why

It was felt that the existing code had become more difficult to understand and maintain over time; and did not align with how the Design System team now prefers to author JavaScript.

Also fixes #2042.

Changes

Refactored JavaScript

The JavaScript has been heavily refactored. It's a little more verbose and there is definitely some repetition that could be reduced (getMobileTab and getDesktopTab are almost identical functions), but it's hopefully a lot easier to understand now! (And it actually minifies to less code than the previous version. 3351 bytes for old vs. 2835 bytes for new, when piped through Uglify!)

Classes are no longer defined as variables at the top of the file. Many of these classes were only used once, and it quickly became tedious having to keep looking up what elements/classes a piece of code was actually referring to. Personally, I think the code is easier to understand with the classes written in-line.

Terminology changes

I've tried to make the terminology more clearly reflect the actual role those elements play. I've only made these naming changes in the JS, though it would probably be sensible to port them—or something similar—over to HTML/CSS too.

Old name	New name
tabsItem	desktopTabs
headingItem	mobileTabs
tabContainer	panel

HTML and CSS tweaks

There are a few minor differences to the HTML and CSS, outside of the refactoring efforts.

• There are various whitespace-only changes to the example macro so that Nunjucks code is indented in the same way as HTML, to try and make it easier to read.
• Panels were previously hidden using a CSS class. This now uses the hidden HTML attribute instead.
• The setting of aria-hidden has been removed (redundant as hidden removes it from the accessibility tree anyway).
• Setting the open Nunjucks parameter would previously apply additional classes to several elements. This has been reduced to a single data-* attribute (data-open) set on the module container. This doesn't strike me as a breaking change as the original classes were only used for detection by JavaScript, and served no other stylistic or functional purpose.

Test changes

• One of the Jest tests associated with these tabs was attempting to reference an element with the ID example-section-headings-open on the Question Pages pattern page. No such element exists. It's been replaced with section-headings-question-pages-example-open, which seems to be the equivalent in the current version of the page.
• The two tests that check for the non-existence of elements have been changed from expect.toBeFalsy() to expect.toBeNull(), as this better describes the expected outcome.

Not changed

Using the design system's tabs component

This refactor does not make use of the equivalent design system component as discussed in #385, due to large differences in mobile functionality and the ability to entirely collapse tab panels. This is refactoring, not revolution!

open and id

Setting the open Nunjucks parameter currently modifies the id used throughout the macro.

I couldn't see any obvious reason for this to be happening, barring some legacy reason, but have not changed it as it's (mostly) harmless and may break tests and some direct links to specific examples.

[querkmachine]

Help wanted

I encountered several situations where I was getting a this is not defined error, when trying to use it within the nodeListForEach polyfill, I assume because the callback function for nodeListForEach overrides the value of this.

I've worked around this by using var self = this at the top of the functions where I had these problems, but this always feels a little hacky. I could avoid this by using arrow functions, but I've (perhaps unnecessarily?) kept to the pre-ES2015 stylings of the existing JavaScript.

If there's a nicer way of dealing with those (I tried some .bind(this) action, but was unsure how to use it in combination with nodeListForEach), I'm all ears!

[netlify]

✅ You can preview this change here:

Name	Link
🔨 Latest commit	5ddacc5
🔍 Latest deploy log	https://app.netlify.com/sites/govuk-design-system-preview/deploys/62d7df26bdcd120009e413e4
😎 Deploy Preview	https://deploy-preview-2242--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 settings.

[querkmachine]

Edit: This has now been resolved.

---

There are some confusing, persistantly failing tests going on here (in addition to some spontaneously failing ones).

They look like this:

 FAIL  __tests__/tabs.test.js
  ● Patterns page › when JavaScript is available › when "hideTab" parameter is set to true › the tab list is not rendered





  ● Patterns page › when JavaScript is available › when "hideTab" parameter is set to true › close button is not shown on the code block

Supposed fails, but with no information as to what or how they've failed.

Reading into the tests themselves, the first one is querying for the non-existence of this element on the "Question pages" pattern: #example-section-headings-open .app-tabs.

However, an element with that ID doesn't exist, not even on the live version of the page, so this test should (theoretically) always pass. The closest element that does exist is named #section-headings-question-pages-example-open, which also doesn't have a child named .app-tabs. Swapping out the ID in the test doesn't rectify the failing test locally, either.

The second test is checking to make sure that the .js-tabs__container--no-tabs .js-tabs__close element does not exist. Similarly to the first, this element does not exist. Unlike the first, the parent element does exist on both the local and live versions of the page. This test should also be always passing, but it's not.

What a mystery!

[vanitabarrett]

This refactor looks really good, it's a lot easier to understand! 🎉

For the var self = this issue, I don't have any suggestions, I'm afraid. Might be worth asking around the devs. I know @36degrees has helped me out in the past with .bind stuff, so may be able to help here?

[36degrees]

One thing I noticed from testing, otherwise this is looking good.

[36degrees]

This appears to be an issue with the existing JavaScript as well, but if you follow the link to the Nunjucks options from another component (e.g. https://deploy-preview-2242--govuk-design-system-preview.netlify.app/components/error-message/#options-error-message-example) on mobile you get a weird open state with a double border between the tab and the panel:

I think this is because the app-tabs__heading--current isn't being added to the tab?

Given this is an existing issue, if this isn't trivial to fix then let's leave it for now.

[querkmachine]

Wasn't aware of that one specifically, but I did notice that the options-table.js file seems to only account for the desktop tabs. It'd be a nice candidate to for refactoring itself, especially if it becomes possible to toggle the details/tabs state programatically rather than setting classes/attributes manually.