Add tabs component #776

alex-ju · 2018-06-08T11:42:04Z

Review page

https://govuk-frontend-review-pr-776.herokuapp.com/components/tabs

Contribution log

alphagov/govuk-design-system-backlog#100 (comment)

Browser and AT testing

Chrome 67

Safari 11

Firefox 60 with inverted colours

IE11

IE10

IE9

IE8

NVDA with Firefox

Tab control. 'Past day' tab, selected, 1 of 4.

JAWS with IE11

'Past day' tab, selected, 1 of 4.

VoiceOver on Safari

'Past day', selected, tab, 1 of 4.

Progress

Update to follow the SCSS styleguide and uses govuk-frontend varibles and mixins
Define example in the yaml schema
Add Nunjucks macro and unit tests for the markup
Update to follow the JavaScript styleguide
Add polyfills for less-capable browser support or has a good fallback
Integrations tests for the JavaScript
Rebase and adjust based on recent govuk-frontend changes (i.e. mixin names and variables)
Update example
A new round of AT testing

Trello card

NickColley · 2018-06-08T12:25:18Z

src/components/tabs/tabs.js

+  var id = $panel.id
+  $panel.id = ''
+  this.changingHash = true
+  window.location.hash = this.getHref($tab).slice(1)


Are we sure we want to be adding the history stack?

User needs:

As a user I want to be able to copy and paste the URL and have tabs show in the same way when I go to this URL

As a user I want to be able to go back to the previous page, despite how many tabs I've interacted with.

With this in mind, I recommend we consider using replaceState

adamsilver · 2018-06-08T13:40:07Z

src/components/tabs/tabs.js

+  if (typeof window.matchMedia === 'function') {
+    this.setupResponsiveChecks()
+  } else {
+    this.setup()


Previously, this feature detect was around the whole component which meant that on small screens no tabs will be setup.

Am I right in thinking that in browsers that don't support matchMedia, they'll get tabs regardless of screen size?

Given that the mobile browsers that we support all have good support for matchMedia maybe this isn't too much of a worry.

With the assumption that most of the time IE9 and below are used by users on desktop machines with wide screens?

Yes, it's probably okay. Just to note two examples where it's not as robust:

An IE9 user who opens up two windows side by side will get tabs in a small-ish screen.

Similarly, Blackberry 7, for example, will get tabs on their small screen.

In both cases, they will experience some layout issues such as horizontal scrolling or wrapping of tabs.

joelanman · 2018-06-08T14:39:05Z

Just to note I think the example needs changing - this example is a pattern that does not have clear labels, and is no longer used by gov.uk

The labels issue was brought up in the working group review: alphagov/govuk-design-system-backlog#100 (comment)

timpaul · 2018-06-15T13:59:03Z

Hey @joelanman - how about this example from the Judicial Case Manager?:

(the respondent tab has a similar table in it)

joelanman · 2018-06-15T17:07:22Z

It's ok, I'm sure in context for these users that the labels are clear, but it would be nice if we could find one where the labels are just clear to most people.

timpaul · 2018-06-18T08:51:32Z

@joelanman - here is a fictional but I think plausible alternative:

Each tab has a similar table, but the numbers are different.

Incidentally (@adamsilver you might have an opinion on this) - do we think that each tab should have a section heading? I don't think the above example needs them, but they might be necessary for accessibility / non-JS...

adamsilver · 2018-06-18T09:59:05Z

I don't think the Tab component needs to dictate whether each panel should be marked up with a heading or not. If headings are needed, then they'll be added to the HTML. If not, they won't be.

The Tab component itself does provide an accessible label (labelled by the panel's related tab label) so that's non-sighted users sorted.

If there's a scenario whereby we want headings without JS but no (visual) headings with JS, then I'd imagine the heading would be given a class of js-hidden which provides that functionality?

kr8n3r · 2018-06-18T15:40:37Z

src/components/tabs/README.md

+
+## Introduction
+
+Component for conditionally revealing content, using the details HTML element.


not sure this is right

good spot, thanks!

kr8n3r · 2018-06-18T15:43:08Z

src/components/tabs/index.njk

+    ],
+    [
+      {
+        text: 'item.id'


we changed the way we show item properties from item.id to items.{}.id

Updated all the item properties and borrowed he explanations for consistency.

36degrees

Not had a chance to go through the tests yet.

36degrees · 2018-06-19T13:43:48Z

src/components/tabs/_tabs.scss

+  }
+
+  // JavaScript enabled
+  .js-enabled .govuk-tabs {


To try and keep specificity to a minimum, should this just be .js-enabled?

36degrees · 2018-06-19T13:47:05Z

src/components/tabs/tabs.js

+  this.checkMode(this.mql)
+}
+
+Tabs.prototype.checkMode = function (mql) {


mql is passed in but never used - within the function we use this.mql.

36degrees · 2018-06-19T13:47:44Z

src/components/tabs/tabs.js

+Tabs.prototype.setupResponsiveChecks = function () {
+  this.mql = window.matchMedia('(min-width: 40.0625em)')
+  this.mql.addListener(this.checkMode.bind(this))
+  this.checkMode(this.mql)


Passing this.mql unnecessarily as checkMode never uses it.

36degrees · 2018-06-19T13:50:41Z

src/components/tabs/tabs.js

+
+    // Remove old active panels
+    this.unhighlightTab($tab)
+    this.hidePanel($tab)


Can we change these two calls for hideTab which wraps them both up together?

36degrees · 2018-06-19T13:51:10Z

src/components/tabs/tabs.js

+  var $activeTab = this.getTab(window.location.hash)
+  if ($activeTab) {
+    this.highlightTab($activeTab)
+    this.showPanel($activeTab)


Likewise I believe both highlightTab and showPanel can be replaced by showTab?

36degrees · 2018-06-19T13:56:08Z

src/components/tabs/tabs.js

+    this.changingHash = false
+    return
+  }
+  var $hashTab = this.getTab(window.location.hash)


Can use hash rather than window.location.hash.

Would also consider skipping hasTab and storing and using the result from getTab which would mirror what we do in setup (var $activeTab = this.getTab(window.location.hash))

36degrees · 2018-06-19T13:57:10Z

src/components/tabs/tabs.js

+  }
+  var $hashTab = this.getTab(window.location.hash)
+  var $currentTab = this.getCurrentTab()
+  if ($hashTab) {


This must be true if we got here? Because we can only reach this code if we didn't return as a result of !this.hasTab.

36degrees · 2018-06-19T13:58:23Z

src/components/tabs/tabs.js

+    this.showTab($hashTab)
+    $hashTab.focus()
+  } else {
+    var $firstTab = this.$tabs.firstElementChild


I think this is dead code that can be removed, but if not it'd be good to reduce the duplication.

Not dead code, as when there's not hash it falls back to the first tab. Will try and reduce the logic.

36degrees · 2018-06-19T13:58:54Z

src/components/tabs/tabs.js

+}
+
+Tabs.prototype.hasTab = function (hash) {
+  return this.$module.querySelector(hash)


Is there a reason this uses a different selector to getTab?

36degrees · 2018-06-19T14:00:40Z

src/components/tabs/tabs.js

+
+function Tabs ($module) {
+  this.$module = $module
+  this.$tabs = $module.querySelectorAll('.govuk-tabs__tab')


Have we tested having multiple tab components on a single page?

Yes, did tested with multiple per page.

alex-ju · 2018-06-19T16:01:48Z

Updated addressing all comments.

36degrees · 2018-06-20T11:44:36Z

src/components/tabs/tabs.js

+Tabs.prototype.createHistoryEntry = function ($tab) {
+  var $panel = this.getPanel($tab)
+  var id = $panel.id
+  $panel.id = ''


Why do we have to null and restore the panel's id? If this is behaviour we definitely need, can we add a comment to explain why?

It is so the page doesn't jump when a user clicks a tab (which changes the
hash).

adamsilver · 2018-06-20T12:09:05Z

src/all.js

@@ -31,6 +32,11 @@ function initAll () {
  nodeListForEach($radios, function ($radio) {
    new Radios($radio).init()
  })
+
+  var $tabs = document.querySelectorAll('[data-module="tabs"]')


At the moment the macro template has the data attribute on the container. This means that if I use the macro, it will automatically get instantiated into tabs.

This could become problematic if I want to do something more custom. Like listen to an event fired by the instance (I know we can't do that right now).

Is GOV.UK Frontend continuing to instantiate automatically like this?

Your concern is valid, but this is consistent with how the other components are being initialised. Will make a note and look into that for all the components.

Yep, it might be okay, broadly speaking, for say tabs and conditional radio button reveals. But as a general approach it can be overly constraining.

Thanks @alex-ju

This use case is already accounted for: you can avoid initAll() and call the constructor yourself.

Since we did work to stop initAll() from executing automatically :)

Yes, not initialising or initialising separately a specific component is also an option.
When Adam mentioned 'something custom', I was thinking of a case when you want to initialise but hook different listeners or add custom functionality to a Tabs instance before doing that, which I think is still possible with the current model:

var myTabs = new Tabs($tabs) myTabs.myCustomFunction = function () { ... } myTabs.init()

adamsilver · 2018-06-20T12:11:03Z

Will the Tabs constructor be a global? Or will it be assigned as GOVUK.Tabs?

Co-authored-by: adamsilver <adambsilver@gmail.com>

alex-ju · 2018-06-20T14:28:18Z

@adamsilver: the Tabs constructor is attached to a GOVUKFrontend global variable and every tab instance will be initialised separately using the new operator.

For example if we consider $tabs a queried tab element, the JavaScript initialisation will be:
new GOVUKFrontend.Tabs($tabs).init()

36degrees

LGTM! 👍📑

NickColley reviewed Jun 8, 2018

View reviewed changes

adamsilver reviewed Jun 8, 2018

View reviewed changes

alex-ju force-pushed the add-tabs-component branch 2 times, most recently from 91ca602 to d830559 Compare June 8, 2018 15:45

alex-ju force-pushed the add-tabs-component branch 2 times, most recently from a2bdc29 to 720371d Compare June 18, 2018 14:29

alex-ju changed the title ~~[WIP] Add tabs component~~ Add tabs component Jun 18, 2018

kr8n3r reviewed Jun 18, 2018

View reviewed changes

alex-ju force-pushed the add-tabs-component branch 3 times, most recently from 69375e7 to d380465 Compare June 19, 2018 10:00

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-776 June 19, 2018 10:32 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-776 June 19, 2018 13:25 Inactive

alex-ju force-pushed the add-tabs-component branch from 099f400 to 8f3983b Compare June 19, 2018 13:27

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-776 June 19, 2018 13:27 Inactive

36degrees reviewed Jun 19, 2018

View reviewed changes

alex-ju force-pushed the add-tabs-component branch from 8f3983b to 66d8a06 Compare June 19, 2018 15:08

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-776 June 19, 2018 15:09 Inactive

alex-ju force-pushed the add-tabs-component branch from 66d8a06 to e8b32f6 Compare June 19, 2018 15:33

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-776 June 19, 2018 15:33 Inactive

alex-ju force-pushed the add-tabs-component branch from e8b32f6 to 57a4a48 Compare June 19, 2018 15:41

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-776 June 19, 2018 15:41 Inactive

36degrees added this to the v1.0.0 milestone Jun 20, 2018

alex-ju force-pushed the add-tabs-component branch from 57a4a48 to 0238a7f Compare June 20, 2018 11:38

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-776 June 20, 2018 11:38 Inactive

36degrees reviewed Jun 20, 2018

View reviewed changes

adamsilver reviewed Jun 20, 2018

View reviewed changes

alex-ju force-pushed the add-tabs-component branch from 0238a7f to ad9675a Compare June 20, 2018 14:00

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-776 June 20, 2018 14:00 Inactive

alex-ju and others added 6 commits June 20, 2018 15:01

Add tabs component

b29b698

Co-authored-by: adamsilver <adambsilver@gmail.com>

Add readme for tabs component

f5d1af4

Add JavaScript for tabs component

a5ccae8

Co-authored-by: adamsilver <adambsilver@gmail.com>

Add classList polyfill

de88b1d

Import tabs component

22edd64

Update changelog

7fe359e

alex-ju force-pushed the add-tabs-component branch from ad9675a to 7fe359e Compare June 20, 2018 14:17

govuk-design-system-ci deployed to govuk-frontend-review-pr-776 June 20, 2018 14:17 View deployment

36degrees approved these changes Jun 20, 2018

View reviewed changes

alex-ju merged commit 1b9ca1c into master Jun 20, 2018

alex-ju deleted the add-tabs-component branch June 20, 2018 15:26

This was referenced Jun 21, 2018

Release v1.0 #809

Closed

Release v1.0.0 #810

Merged


		## Introduction

		Component for conditionally revealing content, using the details HTML element.

Add tabs component #776

Add tabs component #776

Conversation

alex-ju commented Jun 8, 2018 • edited Loading

Review page

Contribution log

Browser and AT testing

Progress

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

joelanman commented Jun 8, 2018

timpaul commented Jun 15, 2018

joelanman commented Jun 15, 2018

timpaul commented Jun 18, 2018

adamsilver commented Jun 18, 2018 • edited Loading

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

36degrees 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju Jun 20, 2018 • edited Loading

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

adamsilver commented Jun 20, 2018 • edited Loading

alex-ju commented Jun 20, 2018 • edited Loading

36degrees left a comment

Choose a reason for hiding this comment

alex-ju commented Jun 8, 2018 •

edited

Loading

adamsilver commented Jun 18, 2018 •

edited

Loading

alex-ju Jun 20, 2018 •

edited

Loading

adamsilver commented Jun 20, 2018 •

edited

Loading

alex-ju commented Jun 20, 2018 •

edited

Loading