Add GOVUK.ShowHideContent to toolkit, introduce tests #305

colinrotherham · 2016-08-10T14:17:44Z

I've seen the ShowHideContent() component used by various service teams (including the prototype kit) but it's often just pasted into application.js where required.

One use is to add progressive disclosure support (reveals a hidden panel) to question and answer forms. For example, answering "Yes" to a question may ask for more information.

It's not ideal to have it copy & pasted around without tests. Copied code will become stale.

For reference, I've added the original (untested) version here:
https://gist.github.com/colinrotherham/b9d53d0ea51bde85d8a1f26231038479

I've added tests and rewritten the module into the latest format for this pull request.

I noticed @gemmaleigh and @fofr discussed it here:
alphagov/govuk_elements#183

Can it live here in this repo instead? If not, please advise where it should go!

Thanks

colinrotherham · 2016-08-10T14:19:10Z

One example that shows testing is needed was that "out of the box" the component doesn't add the necessary aria-controls and aria-expanded attributes on page load.

These are only added when a radio or checkbox is clicked.

colinrotherham · 2016-08-10T14:29:47Z

I noticed @NickColley recently moved selection-buttons.js over to this repo, I presume it would assist to do the same thing with this module too?

Here it is, untested, in the prototype kit:
https://github.com/alphagov/govuk_prototype_kit/blob/master/app/assets/javascripts/application.js

tombye · 2016-08-18T11:08:00Z

javascripts/govuk/show-hide-content.js

+        $content.addClass('js-hidden');
+        $content.attr('aria-hidden', 'true');
+
+        // If the controlling input, update aria-expanded


If $control isn't a controlling input, it looks like the call to getToggledContent at the top of this function wouldn't work as its parent label wouldn't have a reference to the content id. If that's true, is it worth doing the check for aria-controls early and returning if it's not found? Could also be check done outside of this function.

Hi @tombye, whilst it might not be a controlling input (i.e. can't open a show/hide panel), clicks on other radio inputs with the same name attribute are still allowed to hide the show/hide panel.

The controlling input will be told to close the panel via the loop on line 81: https://github.com/alphagov/govuk_frontend_toolkit/pull/305/files/eccbfa8054f812321adadde10bd14be50d34491e#diff-5fcf67c331373114554826652b07e141R81

You're right though. In that, if only one radio in the loop has show/hide content, I was happy for the others to silently be skipped over and not do anything.

I think I just found it odd reading that inputs without an 'aria-controls' attribute or a parent label with a data-target attribute were being sent to getToggledContent which assumes either of those.

At the moment this error is being absorbed by jQuery but it seems like this could be avoided by just filtering the radios down so only those controlling content had their content hidden.

No worries @tombye, did you see the original code I linked to in my opening comments?

The new code sticks to the theme of the original which also looped all radios and also tried to find the show/hide content for them all too.

With a pull request I never want to step on anyone's toes. If you'd like to change this in the future it's all yours! 😊

I saw the code, thanks for that, it helped explain the flow nicely.

You're not stepping on anyone's toes. You put together a clean, well documented and tested piece of code on your own time for the benefit of the community. Please don't think any part of my review ignores the effort or thought that went in 😊

I'm happy to put any suggested changes together into a separate pull request 👍

Brill. Thanks @tombye.

I always imagine if I change too much it won't get merged in!

The goals I had were to:

Reduce fragmentation (copying between other services)

Switch to event delegation (reduces handler overhead on large forms)

Reduce duplicated code between radios and checkbox flows

Check for already-toggled content on first run (new feature)*

Add test coverage

* Added here: https://github.com/alphagov/govuk_frontend_toolkit/pull/305/files/eccbfa8054f812321adadde10bd14be50d34491e#diff-5fcf67c331373114554826652b07e141R120

Lastly, if the ES5 Function.prototype.bind method was available, we could optimise this bit too:
https://github.com/alphagov/govuk_frontend_toolkit/pull/305/files#diff-5fcf67c331373114554826652b07e141R115

Are there any thoughts about adding the ES5 shims to bring .bind(), .trim(), .forEach(), Object.keys() etc to IE8 and other older browsers?

Anyway, I can see my work has been branched off and progressed here: https://github.com/alphagov/govuk_frontend_toolkit/tree/add-govuk-show-hide-content-js

Feel free to close this pull request.

We include a .bind() shim in this repo which is used in the selection buttons code:

govuk_frontend_toolkit/javascripts/govuk/selection-buttons.js

Line 14 in 6d55f22

}.bind(this));

@edds wrote a nice pull request explaining the merits of adding it here:

#120

I can see more shims being adopted if the pros and cons are explained as is done here. Adding .bind() definitely made the selection buttons code a lot clearer.

Ah fab, sorry I couldn't see one at first glance. Thanks @tombye.

tombye · 2016-08-18T11:18:58Z

Hi @colinrotherham, thanks for taking the time to raise this.

I've added a few comments to the code based on my understanding of it from reading through the JS. Apart from those on the docs, none of them are real blockers but I'd be interested in what you think.

Related PR for the front end toolkit: alphagov/govuk_frontend_toolkit#305 Related PR for elements: alphagov/govuk_elements#287

NickColley · 2016-08-19T09:47:00Z

javascripts/govuk/show-hide-content.js

+    var selectors = {
+      namespace: 'ShowHideContent',
+      radio: '.block-label input[type="radio"]',
+      checkbox: '.block-label input[type="checkbox"]'


We might want make sure we're not using presentational classes for binding, this will make this module more flexible.

(https://github.com/alphagov/styleguides/blob/master/js.md#html-class-hooks)

For example

<label class="block-label js-toggle-showhide" data-toggle-target="show-me"> <input type="radio" name="enabled" value="yes" /> Yes </label> <div id="show-me" class="panel js-hidden"> <p>Show/Hide content to be toggled</p> </div>

Hi @NickColley, don't forget this isn't my module. I've only refactored, tested and migrated it here.

If you want to change its behaviour or styling let's open another pull request. It's already in use elsewhere using this class so we'd have to think about backwards compatibility.

@NickColley For example, I've spotted you're also lacking a role="region" on the show-hide div and aren't passing .focus() when the content is toggled open/closed.

Just mentioned this to @gemmaleigh.

If you think I'm the right person to progress with this I'm happy to help?

Hi @colinrotherham, thanks for taking the time to add this JS and the tests and documentation to accompany it. We'll happily take it from here, rather than pester you with change requests. Thanks for your help!

Avoid DOM parentNode traversal (after init, `aria-controls` attribute can be used instead).

gemmaleigh · 2016-08-23T07:16:38Z

I'm closing this in favour of #315.

I'd like to get #315 merged as this will work as-is and is an improvement on what is currently in application.js in GOV.UK elements, anything else should be raised as a new PR.

Thanks everyone! @colinrotherham @tombye @NickColley.

colinrotherham changed the title ~~Add GOVUK.ShowHideContent to toolkit~~ Add GOVUK.ShowHideContent to toolkit, introduce tests Aug 11, 2016

Add GOVUK.ShowHideContent

eccbfa8

gemmaleigh mentioned this pull request Aug 18, 2016

[RFC] Lint codebase using standard alphagov/govuk-prototype-kit#224

Merged

2 tasks

tombye reviewed Aug 18, 2016
View reviewed changes

gemmaleigh added a commit to alphagov/govuk_accessibility_sandbox that referenced this pull request Aug 18, 2016

Add show-hide-content.js

0150884

Related PR for the front end toolkit: alphagov/govuk_frontend_toolkit#305 Related PR for elements: alphagov/govuk_elements#287

gemmaleigh mentioned this pull request Aug 18, 2016

Add govuk showhide content alphagov/govuk_accessibility_sandbox#100

Merged

NickColley reviewed Aug 19, 2016
View reviewed changes

colinrotherham added 2 commits August 19, 2016 13:56

Add clarity to GOVUK.ShowHideContent docs

af50f1a

Slight performance tweak to GOVUK.ShowHideContent

790cc1a

Avoid DOM parentNode traversal (after init, `aria-controls` attribute can be used instead).

gemmaleigh mentioned this pull request Aug 22, 2016

Add GOVUK.ShowHideContent JavaScript #315

Merged

gemmaleigh closed this Aug 23, 2016

colinrotherham deleted the show-hide-content branch September 15, 2016 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GOVUK.ShowHideContent to toolkit, introduce tests #305

Add GOVUK.ShowHideContent to toolkit, introduce tests #305

colinrotherham commented Aug 10, 2016 •

edited

Loading

colinrotherham commented Aug 10, 2016 •

edited

Loading

colinrotherham commented Aug 10, 2016 •

edited

Loading

tombye Aug 18, 2016 •

edited

Loading

colinrotherham Aug 19, 2016

tombye Aug 19, 2016

colinrotherham Aug 19, 2016

tombye Aug 22, 2016 •

edited

Loading

colinrotherham Aug 22, 2016 •

edited

Loading

tombye Aug 22, 2016

colinrotherham Aug 22, 2016

tombye commented Aug 18, 2016

NickColley Aug 19, 2016

colinrotherham Aug 19, 2016 •

edited

Loading

colinrotherham Aug 19, 2016

gemmaleigh Aug 19, 2016

gemmaleigh commented Aug 23, 2016

Add GOVUK.ShowHideContent to toolkit, introduce tests #305

Add GOVUK.ShowHideContent to toolkit, introduce tests #305

Conversation

colinrotherham commented Aug 10, 2016 • edited Loading

colinrotherham commented Aug 10, 2016 • edited Loading

colinrotherham commented Aug 10, 2016 • edited Loading

tombye Aug 18, 2016 • 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

tombye Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

colinrotherham Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombye commented Aug 18, 2016

Choose a reason for hiding this comment

colinrotherham Aug 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemmaleigh commented Aug 23, 2016

colinrotherham commented Aug 10, 2016 •

edited

Loading

colinrotherham commented Aug 10, 2016 •

edited

Loading

colinrotherham commented Aug 10, 2016 •

edited

Loading

tombye Aug 18, 2016 •

edited

Loading

tombye Aug 22, 2016 •

edited

Loading

colinrotherham Aug 22, 2016 •

edited

Loading

colinrotherham Aug 19, 2016 •

edited

Loading