Allow an optional hint for each radio and checkbox item #846

kr8n3r · 2018-06-29T07:13:37Z

It's currently it is not possible to use hints in items as seen in https://www.gov.uk/log-in-file-self-assessment-tax-return/sign-in/prove-identity in Nunjucks macros

This work allows for optional hint object to be passed to each item that outputs a hint component. Id of that hint component is set as aria-describedby on radio/checkbox into so that it's read by AT.

Update template to allow for hint component to be passed
Add example to yaml
Add test to check that hint is present
Update table of arguments with the new option
Regenerate README

Direct urls:
https://govuk-frontend-review-pr-846.herokuapp.com/components/checkboxes/with-hints-on-items/preview
https://govuk-frontend-review-pr-846.herokuapp.com/components/radios/with-hints-on-items/preview

Browser tests:

Chrome 67 and Safari 11.1.1 ✅

IE8 ✅

IE11 ✅

Safari 9.3 iOS and Samsung Internet browser ✅

AT tests:

VoiceOver on maOS

JAWS 16, 17, 18 in IE11: Hint read
NVDA: Hint read
Talkback on Android: Hint read
Voiceover on iOS: Hint read

This fixes: #768

36degrees

Looking good – a few minor comments. Most of the comments I made on the checkboxes component also apply to the radios component.

36degrees · 2018-07-05T15:20:27Z

src/components/checkboxes/checkboxes.yaml

+      - name: gateway
+        id: government-gateway
+        value: gov-gateway
+        html: <b>Sign in with Government Gateway</b>


Can we remove the <b> styling from this and use text so that the example reflects how it'll look 'by default'?

36degrees · 2018-07-05T15:21:09Z

src/components/checkboxes/_checkboxes.scss

@@ -10,6 +10,7 @@
 @include govuk-exports("govuk/component/checkboxes") {
  $govuk-checkboxes-size: govuk-spacing(7);
  $govuk-checkboxes-label-padding-left-right: govuk-spacing(3);
+  $govuk-checkboxes-hint-padding-left-right: $govuk-checkboxes-label-padding-left-right;


Do we need this, or can we just reuse $govuk-checkboxes-label-padding-left-right? Are there valid use cases where we're not going to want these to be the same?

36degrees · 2018-07-05T15:23:09Z

src/components/checkboxes/template.njk

@@ -48,18 +48,30 @@
    {% set id = item.id if item.id else idPrefix + "-" + loop.index %}
    {% set name = item.name if item.name else params.name %}
    {% set conditionalId = "conditional-" + id %}
+    {% set isItemHint = true if item.hint.text or item.hint.html %}


What about hasHint for this variable? I find isItemHint confusing because it suggests that the item is a hint.

36degrees · 2018-07-05T15:24:58Z

src/components/checkboxes/template.test.js

@@ -255,6 +255,36 @@ describe('Checkboxes', () => {
    })
  })

+  it('render hint text', () => {
+    const $ = render('checkboxes', {


I'd make this as short as possible to make it clear exactly what it is we're testing in this component - there's a lot going on in this test which isn't actually required for the test to pass. I also don't think there's a need to use 'realistic' examples here, they'll only go out of date.

I figure checking for input and hint relationship is correct so that it gets announced might be worth testing.
I could just test that it renders, but that not that useful.

It's definitely useful to test that, but there's still a lot going on here for one test:

You could take out a lot of the lines here (for example the entire first item) and the test would still pass, which makes it harder for others coming in to try and understand what's actually being tested.

There are three assertions being made, two of which aren't covered by the test description. If this test were to fail because the aria-describedby isn't what we expect, the description doesn't help you to understand why it's failing.

If you look at e.g. https://github.com/alphagov/govuk-frontend/blob/master/src/components/input/template.test.js#L85-L115:

there's only one assertion per test

the description of each test describes the assertion that's being made

we're asserting on the behaviour we actually care about – we don't actually care what the id or aria-describedby values are – as long as the id is included in aria-describedby. For example, if we added another element which we wanted to add to aria-describedby then this test as written would break (incorrectly) and we'd have to come back and fix it.

there's very little you could delete from each test without breaking it, so it's clear what the test is doing.

Does that make sense?

yep. i've updated the tests

36degrees

Looking good – a few minor things it'd be good to address.

36degrees · 2018-07-11T08:37:29Z

src/components/checkboxes/checkboxes.yaml

+      - name: gateway
+        id: government-gateway
+        value: gov-gateway
+        html: Sign in with Government Gateway


Should be text

36degrees · 2018-07-11T08:37:34Z

src/components/checkboxes/checkboxes.yaml

+      - name: verify
+        id: govuk-verify
+        value: gov-verify
+        html: Sign in with GOV.UK Verify


Should be text

36degrees · 2018-07-11T08:39:55Z

src/components/radios/radios.yaml

+        isPageHeading: true
+    items:
+      - value: gateway
+        html: Sign in with Government Gateway


Should be text

36degrees · 2018-07-11T08:39:57Z

src/components/radios/radios.yaml

+        hint:
+          text: You'll have a user ID if you've registered for Self Assessment or filed a tax return online before.
+      - value: verify
+        html: Sign in with GOV.UK Verify


Should be text

36degrees · 2018-07-11T08:40:54Z

src/components/radios/template.test.js

+    describe('when they include a hint', () => {
+      it('it renders the hint text', () => {
+        const $ = render('radios', {
+          idPrefix: 'gov',


Is this required (in all three tests)?

kr8n3r · 2018-07-11T13:49:47Z

updated

hannalaakso

Looks good. Left a couple of small comments.

Tested in the following:
✅Chrome 67 on Mac
✅ Firefox 61 on Mac
✅ Safari 11.1 on Mac
✅ IE8 - IE11 on Win 7
✅ Chrome, FF on Galaxy S9
✅ Samsung Internet on Galaxy S8
✅ Chrome, Safari on iPhone 6

✅ iOS VoiceOver on Safari and iPhone 7
✅Jaws 16, 17, 18 on IE11
✅NVDA on Firefox

hannalaakso · 2018-07-10T10:29:02Z

src/components/checkboxes/template.njk

@@ -48,18 +48,30 @@
    {% set id = item.id if item.id else idPrefix + "-" + loop.index %}
    {% set name = item.name if item.name else params.name %}
    {% set conditionalId = "conditional-" + id %}
+    {% set hasHint = true if item.hint.text or item.hint.html %}


itemHasHint might be slightly more explanatory.

We don't use the item prefix for anything else in this block (e.g. name, not itemName) – do we think this is different?

We use itemHintId below it. But it's not a strongly held view - itemHasHint just feels like it would make it clearer that the hint is for the item, not the component.

Whatever we do, it'd be good to be consistent within this block – id, name and conditionalId aren't prefixed. If we think it's useful to have that prefix, we should do it for all of them.

we have itemHintId because we already have hintId, the whole group's hint

In that case they should probably all be prefixed. It isn't clear from the context why itemHintId is named differently (to avoid clash with the group level hint) 🤔

hannalaakso · 2018-07-10T10:30:55Z

src/components/checkboxes/checkboxes.yaml

+        value: gov-verify
+        html: Sign in with GOV.UK Verify
+        hint:
+          text: You’ll have an account if you’ve already proved your identity with either Barclays, CitizenSafe, Digidentity, Experian, Post Office, Royal Mail or SecureIdentity.


Nit: this could go very quickly out of date.

alex-ju · 2018-07-16T15:54:09Z

I was hoping to have this in the last release. Do you think it will be in v1.2.0?

NickColley · 2018-07-16T16:03:47Z

@alex-ju we can try to do a quicker release if you're blocked by this for sure.

alex-ju · 2018-07-16T16:11:04Z

not a blocker, and even if so I can reproduce it before the release/updating to the latest version, but it's definitely something that we need short-term.

kr8n3r · 2018-07-19T08:21:47Z

CHANGELOG.md

@@ -75,7 +81,7 @@
  `headingLevel: <number>` parameter. Default is `2`.
  ([PR #853](https://github.com/alphagov/govuk-frontend/pull/853))

- Update date input component  
+- Update date input component


can we keep this in here?

Yeah, we need to figure out how to get our editors stripping whitespace consistently, but that should not block this PR

- Update template to allow for hint component to be passed - Add example to yaml - Add test to check that hint is present - Update table of arguments with the new option - Regenerate README

36degrees

💯

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 June 29, 2018 07:14 Inactive

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from bcfe773 to 0732870 Compare June 29, 2018 07:33

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 June 29, 2018 07:33 Inactive

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 0732870 to 8a81d6c Compare June 29, 2018 07:37

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 June 29, 2018 07:38 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 June 29, 2018 11:01 Inactive

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from ce28732 to a94f25f Compare June 29, 2018 11:03

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 June 29, 2018 11:03 Inactive

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from a94f25f to c1c7797 Compare June 29, 2018 11:05

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 June 29, 2018 11:05 Inactive

kr8n3r changed the title ~~WIP: Allow for optional hint each radio and checkbox item~~ WIP: Allow for optional hint for each radio and checkbox item Jun 29, 2018

kr8n3r mentioned this pull request Jul 1, 2018

Allow for an optional divider between radio items #849

Merged

kr8n3r changed the title ~~WIP: Allow for optional hint for each radio and checkbox item~~ Allow for optional hint for each radio and checkbox item Jul 2, 2018

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 2, 2018 08:27 Inactive

36degrees requested a review from dashouse July 2, 2018 08:46

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 7cebdcb to 100797e Compare July 2, 2018 08:55

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 2, 2018 08:55 Inactive

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 100797e to 6735171 Compare July 2, 2018 14:32

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 2, 2018 14:32 Inactive

kr8n3r added feature request User requests a new feature enhancement and removed feature request User requests a new feature labels Jul 2, 2018

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 6735171 to e59e1d9 Compare July 3, 2018 08:52

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 3, 2018 08:52 Inactive

kr8n3r changed the title ~~Allow for optional hint for each radio and checkbox item~~ Allow an optional hint for each radio and checkbox item Jul 4, 2018

kr8n3r added this to the v1.1.0 milestone Jul 4, 2018

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from e59e1d9 to 592d564 Compare July 4, 2018 13:56

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 4, 2018 13:56 Inactive

36degrees reviewed Jul 5, 2018

View reviewed changes

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 592d564 to 554d071 Compare July 5, 2018 19:12

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 6, 2018 08:07 Inactive

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 5c39317 to bad3253 Compare July 10, 2018 15:55

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 10, 2018 15:55 Inactive

36degrees reviewed Jul 11, 2018

View reviewed changes

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from bad3253 to 6ca9f25 Compare July 11, 2018 13:49

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 11, 2018 13:49 Inactive

hannalaakso reviewed Jul 11, 2018

View reviewed changes

kr8n3r removed this from the v1.1.0 milestone Jul 13, 2018

NickColley added feature request User requests a new feature and removed enhancement labels Jul 13, 2018

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 6ca9f25 to 39182d8 Compare July 13, 2018 18:43

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 13, 2018 18:43 Inactive

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 39182d8 to ff7542a Compare July 19, 2018 08:19

kr8n3r commented Jul 19, 2018

View reviewed changes

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from ff7542a to 61faf13 Compare July 19, 2018 10:39

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-846 July 24, 2018 08:30 Inactive

igloosi added 3 commits July 25, 2018 08:47

Allow for optional hint on each checkbox item

3db529d

- Update template to allow for hint component to be passed - Add example to yaml - Add test to check that hint is present - Update table of arguments with the new option - Regenerate README

Allow for optional hint on each radio item

073973d

- Update template to allow for hint component to be passed - Add example to yaml - Add test to check that hint is present - Update table of arguments with the new option - Regenerate README

Document in CHANGELOG

68f8bce

kr8n3r force-pushed the hint-in-radios-and-checkboxes branch from 61faf13 to 68f8bce Compare July 25, 2018 07:52

govuk-design-system-ci deployed to govuk-frontend-review-pr-846 July 25, 2018 07:53 View deployment

36degrees approved these changes Jul 25, 2018

View reviewed changes

kr8n3r added this to the [NEXT] milestone Jul 25, 2018

kr8n3r merged commit d155120 into master Jul 25, 2018

kr8n3r deleted the hint-in-radios-and-checkboxes branch July 25, 2018 10:16

kr8n3r mentioned this pull request Jul 26, 2018

Release v1.2.0 (feature release) #928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow an optional hint for each radio and checkbox item #846

Allow an optional hint for each radio and checkbox item #846

kr8n3r commented Jun 29, 2018 •

edited

Loading

36degrees left a comment

36degrees Jul 5, 2018 •

edited

Loading

36degrees Jul 5, 2018

36degrees Jul 5, 2018

36degrees Jul 5, 2018

kr8n3r Jul 6, 2018 •

edited

Loading

36degrees Jul 10, 2018 •

edited

Loading

kr8n3r Jul 10, 2018

36degrees left a comment

36degrees Jul 11, 2018

36degrees Jul 11, 2018

36degrees Jul 11, 2018

36degrees Jul 11, 2018

36degrees Jul 11, 2018

kr8n3r commented Jul 11, 2018

hannalaakso left a comment

hannalaakso Jul 10, 2018

36degrees Jul 11, 2018

hannalaakso Jul 11, 2018

36degrees Jul 12, 2018

kr8n3r Jul 12, 2018

hannalaakso Jul 12, 2018

hannalaakso Jul 10, 2018

alex-ju commented Jul 16, 2018

NickColley commented Jul 16, 2018

alex-ju commented Jul 16, 2018

kr8n3r Jul 19, 2018

NickColley Jul 19, 2018

36degrees left a comment

Allow an optional hint for each radio and checkbox item #846

Allow an optional hint for each radio and checkbox item #846

Conversation

kr8n3r commented Jun 29, 2018 • edited Loading

36degrees left a comment

Choose a reason for hiding this comment

36degrees Jul 5, 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

kr8n3r Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

36degrees Jul 10, 2018 • edited Loading

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

kr8n3r commented Jul 11, 2018

hannalaakso 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

alex-ju commented Jul 16, 2018

NickColley commented Jul 16, 2018

alex-ju commented Jul 16, 2018

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

kr8n3r commented Jun 29, 2018 •

edited

Loading

36degrees Jul 5, 2018 •

edited

Loading

kr8n3r Jul 6, 2018 •

edited

Loading

36degrees Jul 10, 2018 •

edited

Loading