Add tasklist component #1173

andysellick · 2017-10-26T07:05:57Z

A tasklist is meant to help users achieve a goal on GOV.UK, such as learning to drive. Links to pages of information are presented in a series of steps, each of which containing one or more expandable sections.

based on the accordion code
the JS should be quite similar to the accordion but tasklist does not have the 'last section open remembered in the URL' functionality enabled by default
the JS spec file should be almost identical to the one from accordion, but with an addition for the data-remember attribute for 'last section open remembered in URL' functionality

Large tasklist:

Small tasklist (large should also look like this on mobile):

Component guide link: https://govuk-static-pr-1173.herokuapp.com/component-guide/task_list

Related trello card: https://trello.com/c/o0BCuFub/190-rewrite-the-accordion-to-meet-our-teams-needs

Accessibility and compatibility

This work included a session with the GDS accessibility team, who made a number of recommendations which have since been implemented. It will go into public beta first, during which we'll be testing it with users with accessibility needs.

Tested with IE8 -> 11, Edge latest, Chrome latest and 54, FF latest and 48, Opera latest, Safari latest, Safari 5.1 (Windows), ipad mini 2, iphone 6s, Android Nexus 6.

Tested using Voiceover on Chrome and Safari.

fofr · 2017-10-26T08:28:37Z

This will need a design review.

NickColley · 2017-10-26T09:33:59Z

Can you list what devices/browsers (including assistive tech) this has been tested with?

andysellick · 2017-10-26T10:54:06Z

It's gone through numerous iterations with Kate, including a couple of pairing sessions to tweak things. Mia's also seen it.

alecgibson

This generally looks okay to me. My biggest question is: what's happening with commonisation with the accordion? We now have the same code floating about in a couple of places, and it would be good to graduate the Accordion to Static, and - if possible - even use that component as the basis for this component.

alecgibson · 2017-10-26T10:10:27Z

app/assets/images/icons-plus-minus.svg

@@ -0,0 +1,3 @@
+<?xml version="1.0"?>


Is this stuff all the same as Collections? Can you please make sure you raise a ticket to remove it from Collections (along with things like current-location.js, history-support.js, etc.)?

Funnily enough, I've already started work on a PR in collections that removes this JS, but I've been waiting to get this all approved and merged first. Good point on the images though, hadn't thought of those.

alecgibson · 2017-10-26T13:39:36Z

app/assets/javascripts/govuk-component/tasklist.js

+      }
+    };
+
+    var rememberOpenSection = 0;


(I would personally find this easier to understand if it was var shouldRememberOpenSection = false and then use the !! operator to turn .length into true.)

Good suggestion, thanks, will change.

alecgibson · 2017-10-26T13:41:47Z

app/views/govuk_component/tasklist.raw.html.erb

+<% if steps %>
+  <div
+    data-module="tasklist"
+    class="pub-c-tasklist js-hidden <% if not small %>pub-c-tasklist--large<% end %>"


Could you please prefer ! over not? Or better yet, prefer unless over if !.

Will change to 'unless'. I'm afraid that's my limited Ruby experience showing..

alecgibson · 2017-10-26T14:40:11Z

app/views/govuk_component/tasklist.raw.html.erb

+      <% steps.each_with_index do |step, index| %>
+        <%
+          step_active = false
+          step.each do |this_step|


How do you enumerate on a single step? I find this naming confusing. Why do we have steps -> step -> this_step ?

Also, if this whole loop construct exists to determine step_active, then a more idiomatic way to achieve this is something like:

step_is_active = step.any? do |this_step| this_step[:panel_links].to_a.any? { |s| s[:active] } end

I knew there must be a cleaner way of doing this. Thank you!

alecgibson · 2017-10-26T14:46:15Z

app/views/govuk_component/tasklist.raw.html.erb

+            end
+          end
+        %>
+        <li class="pub-c-tasklist__step <% if step_active %>pub-c-tasklist__step--active<% end %>" <% if step_active %>aria-current="step"<% end %>>


(I personally prefer removing the need for end like so: class="<%= 'pub-c-tasklist__step--active' if step_active' %>")

Again, my Ruby knowledge. That's helpful, thanks. I'm definitely learning..

alecgibson · 2017-10-26T14:48:13Z

app/views/govuk_component/tasklist.raw.html.erb

+  section_count = 0
+%>
+<% if steps %>
+  <div


This whole file is hugely nested. Potentially consider breaking up into smaller partials.

It is a bit. I'll try some extra line spaces first to see if that helps.

andysellick · 2017-10-26T15:36:53Z

Fair comment @alecgibson about the duplication of accordion code. This all relates to alphagov/govuk-rfcs#84, if we move components into static it then breaks integration tests. Basically we chose this approach (creating a new comp in static based on the accordion) because it's the quickest and easiest solution for right now. When the RFC is complete we'll hopefully have a better solution and can then rework the tasklist to fit.

fofr · 2017-10-30T09:35:20Z

app/assets/javascripts/govuk-component/tasklist.js

@@ -0,0 +1,333 @@
+// Most of this is taken from the service manual


Does this JS differ in any way to the accordion component?
Are the differences only in the class names of elements?
If so, can those class names be abstracted somehow?
Is it appropriate to avoid duplication of this JS?
If it does differ – is it possible to write this in a way that it builds on an Accordion module?

It is different (details below) and I think it's going to continue to get more different. In terms of avoiding duplication or building upon the existing accordion, that might be possible except we'd have to separate out the tracking, which is fairly heavily intertwined (the accordion tracks some content clicks based on an assumed content, which isn't ideal - I'm learning more about this all the time).

Differences are:

'remember last section opened in URL hash' functionality disabled by default

new functionality to allow a section to be opened by default

I'm recommending that we remove the first item altogether as it's not needed when the its in the sidebar and having it on the main one only would be inconsistent.

Also, coming soon are some changes to how the GA tracking works.

I might update that comment to be clearer..

The first point

'remember last section opened in URL hash' functionality disabled by default

That feature is still needed for users who browse from a task list to a content page and then hit "Back", the section should remain open so they can continue from where they left off.

Two thoughts about that.

Yes, in an ideal world. But the solution doesn't remember all the accordion sections that were opened, only the last one - so the page might look different when they click 'back' anyway.

Behavioural consistency is important, and we're not planning on having this when it's in the sidebar.

fofr · 2017-10-30T09:37:12Z

app/assets/stylesheets/govuk-component/_tasklist-print.scss

@@ -0,0 +1,20 @@
+// scss-lint:disable SelectorFormat


We need to upgrade the linter on static in a separate PR.

fofr · 2017-10-30T09:38:22Z

app/assets/stylesheets/govuk-component/_tasklist.scss

@@ -0,0 +1,358 @@
+.pub-c-tasklist {


Is it possible to write this in a way that shows what's copied from an accordion, and what's specific to a tasklist?

At the moment I fear it'll be difficult to make parts of this reusable, and too easy for the two patterns to grow out of sync.

I think the tasklist has already deviated significantly from the accordion that keeping them in sync is neither possible or necessarily desirable. In fact, we're currently looking at a design proposal that would make the tasklist look very different. Happy to discuss though as I feel like I'm unaware of the merits involved.

Hmm. I'm also worried that these two look very similar at the moment. At the very least, I'd hope that we could base both components on a "parent" component that encapsulates as much common behaviour as possible (eg expandable sections), and then make it as configurable as we need to meet the needs of both accordions and task lists. I think this is doable if we do more work with blocks, etc.

That would be a good solution. Just to let you know I'm now looking at further changes to how the GA stuff works for task lists, which will further change the code involved.

If tracking turns out to be a pain to commonise, a possible solution is to have no tracking by default, and people will have to implement their own tracking on a per-use basis (which could be done through JS).

That would be great. I think I'd have done that from the start if the tracking on the accordion hadn't been so detailed and complex.

fofr · 2017-10-30T09:39:36Z

app/assets/stylesheets/govuk-component/_tasklist.scss

+
+// scss-lint:disable SelectorFormat
+
+// when js is enabled


You could do .js-enabled … rather than use the comment.

I could, yes. I wonder why it wasn't that way in the original accordion? All I'm doing is changing the class name.

fofr · 2017-10-30T09:46:44Z

app/assets/stylesheets/govuk-component/_tasklist.scss

+      height: $icon-size-large;
+      width: $icon-size-large;
+      margin-top: -($icon-size-large / 2) - 2;
+      // repeating these two lines for the benefit of IE8, which doesn't support background size


Could you indicate why this needs repeating?

Will clarify.

fofr · 2017-10-30T09:50:06Z

app/views/govuk_component/docs/tasklist.yml

@@ -0,0 +1,342 @@
+name: Tasklist


I prefer "Task list", "tasklist" isn't a word

"Task list" and task-list will be consistent with other list components like:
https://government-frontend.herokuapp.com/component-guide/document-list
https://govuk-collections.herokuapp.com/component-guide/taxon-list
https://govuk-collections.herokuapp.com/component-guide/topic-list

fofr · 2017-10-30T09:53:01Z

app/views/govuk_component/docs/tasklist.yml

+name: Tasklist
+description: Numbered steps containing expanding/collapsing sections
+body: |
+  Tasklists are designed to show a sequence of steps towards a specific goal, such as 'learning to drive'. Each step of a tasklist can contain one or more sections. Each section can contain one or more links to pages. Each section is collapsed by default so that users are not overwhelmed with information.


Should we specifically refer to the research about why task lists are collapsed?

NickColley · 2017-10-31T11:51:15Z

You've mentioned this has been tested with VoiceOver, since this is a navigation component that'll be used on a lot of GOV.UK's important journeys, this should be tested with the assistive technologies that are more commonly used.

When will this be put in front of users? Has this been through a lab session with users of assistive tech? Again since this is a complicated piece of UI for an important journey it'd be good to have this.

NickColley · 2017-10-31T12:00:13Z

app/views/govuk_component/task_list.raw.html.erb

@@ -0,0 +1,84 @@
+<%
+  steps ||= false
+  small ||= false


Should this be 'size'? (Not a big deal)

It would be if there were more size alternatives than just 'small'. But at the moment it's just a flag. I think it's unlikely we'll ever have another size (the only option I think would be one bigger than the default, but that seems improbable) but if we do this can be rewritten then.

NickColley · 2017-10-31T12:03:06Z

This is looking good, I agree with Alec, this feels like an uneasy amount of duplication of code.

@andysellick can you record this as techincal debt alongside the static/slimmer discussion?

andysellick · 2017-10-31T13:06:30Z

Recorded - alphagov/govuk-rfcs#84 (comment)

NickColley · 2017-10-31T16:52:17Z

Spoke to @sihugh about it breaking when used in alphagov/frontend.

We talked through three approaches.

Add reset.css to alphagov/frontend
Add additional rules to the component in static to reset default styling at a component basis
Add an additional style sheet called 'shame.css' that includes 2. overrides but only in alphagov/frontend
Seems the most per convention way forward, since components should not rely on global state to work well.

Out of scope: We'd probably want to raise an issue also on static to consider removing reset.css for other components.

andysellick · 2017-10-31T16:59:14Z

Have now tested with a few more screen readers. No obvious accessibility issues, although some interesting observations.

The 'just read the page' option in some of them resulted in less information being presented. JAWS didn't announce the existence of the buttons, NVDA did but didn't say they were collapsed (Voiceover had no problem). However, navigating e.g. with the tab key, no problem. Presumably most users will favour the second option so should be no problem.

- don't rely on a reset being already present in the page, as in some applications it isn't

fofr temporarily deployed to govuk-static-pr-1173 October 26, 2017 07:06 Inactive

andysellick force-pushed the create-tasklist-component branch from 6ebd342 to 06f6531 Compare October 26, 2017 07:07

fofr had a problem deploying to govuk-static-pr-1173 October 26, 2017 07:07 Failure

andysellick force-pushed the create-tasklist-component branch from 06f6531 to 913ad02 Compare October 26, 2017 07:10

fofr temporarily deployed to govuk-static-pr-1173 October 26, 2017 07:10 Inactive

andysellick force-pushed the create-tasklist-component branch from 913ad02 to 319cc4d Compare October 26, 2017 07:12

fofr temporarily deployed to govuk-static-pr-1173 October 26, 2017 07:12 Inactive

andysellick force-pushed the create-tasklist-component branch from 319cc4d to e7bbe5a Compare October 26, 2017 07:17

fofr temporarily deployed to govuk-static-pr-1173 October 26, 2017 07:17 Inactive

andysellick requested review from fofr, NickColley and alecgibson October 26, 2017 07:25

alecgibson reviewed Oct 26, 2017

View reviewed changes

andysellick force-pushed the create-tasklist-component branch from e7bbe5a to 2dfa02d Compare October 26, 2017 14:57

fofr temporarily deployed to govuk-static-pr-1173 October 26, 2017 14:57 Inactive

andysellick force-pushed the create-tasklist-component branch from 2dfa02d to f865ebd Compare October 26, 2017 15:32

fofr temporarily deployed to govuk-static-pr-1173 October 26, 2017 15:32 Inactive

sihugh mentioned this pull request Oct 27, 2017

Add learn-to-drive-a-car task list page alphagov/collections#400

Merged

fofr temporarily deployed to govuk-static-pr-1173 October 27, 2017 12:19 Inactive

fofr reviewed Oct 30, 2017

View reviewed changes

fofr temporarily deployed to govuk-static-pr-1173 October 30, 2017 09:59 Inactive

andysellick force-pushed the create-tasklist-component branch from f865ebd to 652589f Compare October 30, 2017 13:25

fofr temporarily deployed to govuk-static-pr-1173 October 30, 2017 13:25 Inactive

Add tasklist component

5bce774

andysellick force-pushed the create-tasklist-component branch from 652589f to 5bce774 Compare October 30, 2017 13:48

fofr temporarily deployed to govuk-static-pr-1173 October 30, 2017 13:48 Inactive

NickColley reviewed Oct 31, 2017

View reviewed changes

andysellick mentioned this pull request Oct 31, 2017

RFC 84: Replace Static by a gem alphagov/govuk-rfcs#84

Closed

fofr temporarily deployed to govuk-static-pr-1173 November 1, 2017 09:14 Inactive

andysellick force-pushed the create-tasklist-component branch from 4081d9e to a1b16c6 Compare November 1, 2017 09:50

fofr temporarily deployed to govuk-static-pr-1173 November 1, 2017 09:50 Inactive

andysellick force-pushed the create-tasklist-component branch from a1b16c6 to 82cc67d Compare November 1, 2017 09:56

fofr temporarily deployed to govuk-static-pr-1173 November 1, 2017 09:56 Inactive

andysellick force-pushed the create-tasklist-component branch from 82cc67d to b76bc4c Compare November 1, 2017 10:17

fofr temporarily deployed to govuk-static-pr-1173 November 1, 2017 10:17 Inactive

andysellick force-pushed the create-tasklist-component branch from b76bc4c to 05d3b49 Compare November 1, 2017 10:23

fofr temporarily deployed to govuk-static-pr-1173 November 1, 2017 10:23 Inactive

andysellick force-pushed the create-tasklist-component branch from 05d3b49 to cbeb697 Compare November 1, 2017 10:25

fofr temporarily deployed to govuk-static-pr-1173 November 1, 2017 10:25 Inactive

Include reset styles in component

99b153c

- don't rely on a reset being already present in the page, as in some applications it isn't

andysellick force-pushed the create-tasklist-component branch from cbeb697 to 99b153c Compare November 1, 2017 10:27

fofr deployed to govuk-static-pr-1173 November 1, 2017 10:28 View deployment

NickColley approved these changes Nov 1, 2017

View reviewed changes

andysellick merged commit aa85437 into master Nov 1, 2017

andysellick deleted the create-tasklist-component branch November 1, 2017 10:51

		@@ -0,0 +1,333 @@
		// Most of this is taken from the service manual

Add tasklist component #1173

Add tasklist component #1173

Conversation

andysellick commented Oct 26, 2017 • edited Loading

fofr commented Oct 26, 2017

NickColley commented Oct 26, 2017

andysellick commented Oct 26, 2017

alecgibson 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

andysellick Oct 26, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andysellick commented Oct 26, 2017

fofr Oct 30, 2017 • 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

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

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

NickColley commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley commented Oct 31, 2017

andysellick commented Oct 31, 2017

NickColley commented Oct 31, 2017

andysellick commented Oct 31, 2017

andysellick commented Oct 26, 2017 •

edited

Loading

andysellick Oct 26, 2017 •

edited

Loading

fofr Oct 30, 2017 •

edited

Loading