Accordion component

[frankieroberto]

Note for reviewers:

Please leave any comments pertaining to the guidance/documentation or the content used for the accordion on alphagov/govuk-design-system#635.

Please leave any comments for the accordion component itself (design, code) on this PR.

Preview here: https://deploy-preview-635--govuk-design-system-preview.netlify.com/components/accordion/

---

This is work-in-progress on adding an Accordion component to the Design System.

(Opening this Pull Request mainly as a place to collaborate and discuss the component implementation)

So far I've just done the most minimal lift-and-shift from https://github.com/frankieroberto/accordion to get it working, which has meant:

• Renaming this.element to this.$module (following the convention of existing components)
• Moving all the setup code from the constructor (new Accordion()) to an explicit function call (new Accordion().init()), following the convention of existing components.
• Adding a super-basic Nunjucks macro+template (I'm not quite sure why these are separate files but that seems to be the convention).
• Some minimal CSS tweaks to stop the CSS linter complaining 😄

To-do (prior to Working Group review):

• Update the CSS class names to follow Design System conventions
• Update the CSS syntax itself to follow the coding standards.
• Update the javascript to follow the coding standards
• Investigate pros and cons of using the <details> element instead of divs, and the .open DOM API.
• Consider making the SVG image a data-url to avoid a network request?
• Move "+/-" image to markup (see Accordion component #958 (comment))
• Ensure "+/-" image is visible if user changes the default colours
• Add unit tests
• Add integration tests
• Add Nunjucks macro
• Add macro options as yaml, similar to params here
• Browser testing (ongoing)
• Assistive technology testing
• Documentation - draft
• Save state of each accordion to session storage - @injms is working on this
• Update the focus state to be yellow solid outline
• Consider using the same language as W3C who use the terms "accordion header" and "accordion panel", ie call ours "header" and "panel" in the markup, instead of "section header" and "section body"
• Make into a <button> that is inside a <h3>
• Add id that is used by aria-labelledby of relevant panel
• Add aria-controls that refers to id of relevant panel
• Add aria-expanded that is toggled false / true based on state
• Add idto panel that corresponds to aria-controls of relevant header
• Add `role="region" attribute to panel
• Add aria-labelledby to panel that refers to id of relevant header

To-do (can be done after Working Group review)

• Fix "+/-" svg not showing IE9-11
• Fix IE8 issues - see Accordion component #958 (review)
• Add template tests
• Add visually hidden text "panels" to "Open all / Close all" button so it says "Open all / close all panels"
• Make the heading level for the macro configurable by doing something similar to:

  govuk-frontend/src/components/panel/template.njk

  Line 5 in 3e7f8f1

  <h{{ headingLevel }} class="govuk-panel__title">

• Remove HTML default padding from govuk-accordion__section-header-button to leave the text hard aligned to the left edge
• add @include govuk-responsive-margin(6, "bottom"); to .govuk-accordion as all components / wrappers on this "outer" layer have the same bottom margin applied.
• .govuk-accordion__section currently use magic numbers for the spacing. Ideally we should use padding values from the spacing scale (spacing scale 3, 15px) and override with current magic values in a separate section at the bottom of the scss file. This is down to where New Transport sits on the baseline compared to the default sans-serif, we have a new version of this the font that fixes this so it would be great to quickly remove the "magic numbers" block of CSS when this version of the font is released.
• Add padding top and bottom (spacing scale 3, 15px) to .govuk-accordion__section-panel
• Remove the margin bottom from last item inside .govuk-accordion__section-panel we do this on tabs, panel etc using:

& > :last-child {
 margin-bottom: 0;
}

• Add a "safe area" using padding to stop the icon clashing with the additional description when the text is long / on mobile
• Align the icons fully to the right on mobile size, it's less likely the hover state will be seen and will allow more space for the "safe area"
• Change the component to use a description (or summary as we now call it that in the Design System?) option for adding the summary line (at the moment we pass in markup to do this)
• Fix background colour of buttons on Firefox inverted colours view
• Fix colour of button text on Firefox inverted colours view > looks like FF makes text in buttons dark
• Rename panel to content - or vice versa
• Prefix header, heading, summary, content classes with "section"
• Add template tests - IN PROGRESS
• Tidy up no-js classes
• Tidy up Sass in general - nice to have
• Fix failing tests
• Do a team review of public API of component
• Do a team design/guidance review of preview in Design System
• Do a dev team code review of Frontend PR
• Decouple JS classes and style classes - not doing this for now, we'll have to discuss general approach to this with devs
• Should we not print out component in template if user hasn't passed through both heading and content?> we can look at this later
• Final cross browser testing
• Final assistive tech testing

Post working group review / pre-publish work

• Fix bug in second example whereby the h2 inside a h2 causes the heading 2 to be announced twice for the same heading. This and a div as the child of a h2 are not valid HTML.
• The focus state should expand to fill the heading so that it matches the hover state
• Fix "+/-" svg not showing IE9-11 - IN PROGRESS
• Fix IE8 issues - see Accordion component #958 (review) - IN PROGRESS
• SVG is not working on IE8
• Change the component to use a description (or summary as we now call it that in the Design System?) option for adding the summary line (at the moment we pass in markup to do this)
• Fix background colour of buttons on Firefox inverted colours view
• Fix colour of button text on Firefox inverted colours view > looks like FF makes text in buttons dark
• Rename panel to content - or vice versa
• Prefix header, heading, summary, content classes with "section"
• Add template tests - IN PROGRESS
• Tidy up no-js classes
• Tidy up Sass in general - nice to have
• Add visually hidden text "panels" to "Open all / Close all" button so it says "Open all / close all panels"
• Fix failing tests
• Do a team review of public API of component
• Do a team design/guidance review of preview in Design System
• Do a dev team code review of Frontend PR
• Final cross browser testing
• Final assistive tech testing
• Decouple JS classes and style classes - not doing this for now, we'll have to discuss general approach to this with devs
• Should we not print out component in template if user hasn't passed through both heading and content?> we can look at this later
• Should we consider not hardcoding govuk-heading-m in the template? We could: 1. @extend govuk-heading-m in the code (I know we don’t like extends), 2. just manually copy styles over, 3. Pass govuk-heading-m as a macro option to the examples (although then the component wouldn’t look right by default)
• Rebase PR into nice neat commits
• Icon doesn't change on IE8 on expand/collapse

Nice to have

• Consider adding clearer guidance about instantiating multiple accordions. With accordions it seems likely that there would be multiple ones on the page. We just don't have a clear place for where this guidance would live... UPDATE: We're going to stick to initAll() for now which does this. If user wants more fine grained control they'll probably know enough to do this without guidance.
• Finish tests for storing of state with Ian James
• It looks like storestate() loops through all sections when one section is clicked on. It would be more effective just to do the one that’s been clicked on
• Store class names in accordion.js in $module instead of defining them in the methods
• init() in accordion.js has a lot going on, it might be better to break it into separate functions
• accordion.js lines 96-99 about triggering reflow for IE8: is this bit of code necessary? could we document it more?
• Future proof span to button code so that it copies over all attributes and their values from span - we currently don’t set any attributes on span but might do so in the future
• Some general restructuring of accordion.js, it’s hard to follow at times and there's duplication
• Investigate if aria-describedby on button should be there without js
• Make govuk-accordion__section into
  and test with AT that this doesn’t introduce unnecessary noise
• Consider making checkForSessionStorage() into a more generic helper

References:
W3C Authoring Practises - Accordion
W3C Accordion example

[frankieroberto]

Hello. I've not used your recommended unit test framework, Jest, before, but it looks straightforward, so I'm not too worried about that. That said, I'm not sure how much of the Accordion class should be exposed as public properties / functions vs private?

Do you have a standardised way of doing integration testing at all, eg setting up an HTML page, adding the javascript, and then clicking the various elements and asserting what should happen? Selenium?

[frankieroberto]

Just realised that Jest does do full integration testing, so ignore that question...

[frankieroberto]

Do you have any thoughts on how the javascript should be structured. Currently, both Accordion, AccordionSection and all their prototype functions are exposed as public functions. Not only that, but AccordionSection has to be passed an explicit reference to the parent Accordion object (so that it tell the parent object to update the state of the "Open/close all" button when one of the sections is opened/closed.

This feels a bit messy.

An alternative pattern might be something like this:

function Accordion(element) {
  this.sections = [new Section()]

  function Section(sectionElement) {
    this.expanded = false
  }

  Section.prototype.setExpanded = function(expanded) {
    this.expanded = expanded
  };
}

Accordion.prototype.setExpandAll = function(expandAll) {
  for (section of this.sections) {
    section.setExpanded(expandAll)
  }
}

Which would only expose an expanded property on each of the sections, a setExpanded() function on the settings, and a setExpandAll() function on the parent:

var accordion = new Accordion(document.querySelector('.govuk-accordion'))
accordion.sections.length
=> 2
accordion.sections[0].setExpanded(true)
accordion.sections[0].expanded
=> true
accordion.setExpandAll(true)
accordion.sections[1].expanded
=> true

(it'd be nice to be able to avoid having a separate setter function, but I'm not sure how easy that is with javascript right now).

Any thoughts?

[hannalaakso]

Looking good 👍 I've left some comments but haven't had a chance to review most of it yet, will continue to review later. I'm also going to take a look at your above question about structuring JS.

[hannalaakso]

Hi @frankieroberto, as discussed I've pushed some changes to the JS, these address the structure of the JS questions. We can keep discussing this obviously.

@joelanman Do you have any thoughts around using "+ or -" or arrow up/down as we use with details component? Another question: should we consider an enhancement of storing the open/closed state of sections?

[hannalaakso]

This bug means that it's not possible to expand details element with print styles. Until it's fixed we probably shouldn't use details element with accordion.

[joelanman]

On the open/close control:

• Service manual and GOV.UK Pay use a plus/minus and say they didn't find any issues, so didn't try anything else.

• DVSA say they tried plus/minus and up/down arrow and the arrows worked better

• Step by step navigation use quite a different approach. They started with plus/minus. They ended up using the words Show and Hide, on the left under the heading. Being on the left means screen magnifier users can see the control.

I would say from this that we have some evidence that arrows are clearer than plus/minus.

The Step by step approach is probably less ambiguous, with words rather than images, and it has an accessibility advantage with the controls on the left. However it is not as widely used as the other accordion design and so I don't know if we can recommend it for all services.

[frankieroberto]

DfE had an issue with the use of the Back button and the existing accordion, their fix for which is summarised here: DFE-Digital/search-and-compare-ui#156

This makes me think we should do some research into whether/how different browsers restore state when using the back button – we've definitely observed some differences between browsers for this.

[dankmitchell]

We'd also want to ensure that the accordions don't all flash open whilst the javascript is loading

[frankieroberto]

@dankmitchell do the existing ones do that at all?

[dankmitchell]

@frankieroberto some context #1000

[frankieroberto]

I've updated the component now to incorporate this fix from DfE, which makes sure that that the button says "Close all" if the component is initialised with all the sections open: DFE-Digital/search-and-compare-ui@4f129b8 (thanks @tvararu!)

I've also updated the nunjucks macro so that you can now pass in an optional expanded boolean param to any of the items in the list to set their initial state.

[frankieroberto]

@hannalaakso any thoughts on how we should "inline" (via base64 encoding) the SVG image? I think it makes sense to keep the SVG file in the repo, but have the inlining be part of the sass compilation step? In which case we'd need to pick an npm package like https://www.npmjs.com/package/sass-inline-image to add a sass function?

[frankieroberto]

Work-in-progress on the documentation is here: alphagov/govuk-design-system#579

[hannalaakso]

Decisions from catch up with @frankieroberto and @joelanman:

• We’re going to go with +/- for the accordion heading button (as opposed to ^/v) as there are more examples of services using this pattern.

• Re: how to handle the +/- SVG, we currently add the SVG directly into the markup for the header logo and have a fallback PNG in assets for browsers that don't support this so this would be the default way of doing this. The upside is that we can change the colour easily for users who change the default colours. Having the base 64 encoded SVG in Sass could also be an option but it would need to support filling the colour when user changes the colours. @frankieroberto might do some more investigation if he has time.

• We’re going use a light grey hover colour for the accordion heading (focus state will be the standard GOV.UK orange border) - there didn't seem to be a good reason not to have this extra visual indication even though there aren't examples of services using it. It can be marked as requiring more research.

• We're working on draft guidance here.

[hannalaakso]

Addition to the minutes from last week:

We discussed storing of state and decided that since this is something Service Manual accordions use (example here), we should try to get it in if we can (but it shouldn't delay the initial release of the component if it comes to it). We’ll look into using session storage for this.

We can either advise in guidance that service teams will need to make sure each accordion item has a unique ID or we can look into generating a unique ID based on a combination of page url and indexed ID for the accordion item.

[hannalaakso]

Just a note to say that I've done some testing with Jaws and NVDA and have added some accessibility to-do's to the PR description.

[frankieroberto]

@hannalaakso I've made the accessibility changes you suggestion, except for a couple of differences, which I think are worth discussing:

• The section header is an h2 instead of an h3. Possibly we need to make this configurable, but in my experience the accordions headers are usually the second-level heading rather than the third.
• I've used role=button rather than a <button> element within the h2. This is mainly because the javascript has to add the button semantics rather than the nunjucks macro (as we don't want a button to be present if javascript is disabled), and it's a bit easier to add an attribute rather than copying the node, removing it, and adding a replacement with a different element name. However we could still do that, if it proves to be better to have a <button> element rather than an role.

Other comments:

• I've added the role=region to the panel. However I did note that this was optional within the W3C guidance, which also says:

Avoid using the region role in circumstances that create landmark region proliferation, e.g., in an accordion that contains more than approximately 6 panels that can be expanded at the same time.

• should we add extra logic to only add it when there's less than < 6 panels? Or are we confident that in most circumstances, it's better to have the region role than to not?

A lot has changed since my initial review

[aliuk2012]

Non-blocking suggestion,
These two lines and line 60 look like they could do with a function that returns the mark up.

Also if we wanted to add translation support to the component we could use the macro to determine the text and set a html data attribute which this js could pickup and use.

[frankieroberto]

This is looking good, well done everyone who's contributed so far!

[36degrees]

😍👏

[frankieroberto]

🎊