Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update checkboxes and radio buttons to include item hint classes on i… #1648

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
### Fixes

- [Pull request #1655: Ensure components use public `govuk-media-query` mixin](https://github.com/alphagov/govuk-frontend/pull/1655).
- [Pull request #1648: Update checkboxes and radio buttons to include item hint classes on item hint](https://github.com/alphagov/govuk-frontend/pull/1648)
- [Pull request #1638: Check component item arrays are not empty before outputting markup](https://github.com/alphagov/govuk-frontend/pull/1638).

## 3.4.0 (Feature release)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ exports[`Checkboxes when they include a hint renders the hint 1`] = `
>
If you have dual nationality, select all options that are relevant to you.
</span>
<span id="example-item-hint"
class="govuk-hint govuk-checkboxes__hint"
>
Hint for british option here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: capitalise "british".

</span>
<span id="example-3-item-hint"
class="govuk-hint govuk-checkboxes__hint app-checkboxes__hint-other"
data-test-attribute="true"
>
Hint for other option here
</span>
`;

exports[`Checkboxes when they include an error message renders the error message 1`] = `
Expand Down
23 changes: 0 additions & 23 deletions src/govuk/components/checkboxes/checkboxes.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -261,29 +261,6 @@ examples:
- value: other
text: Citizen of another country

- name: with all fieldset attributes
data:
idPrefix: example
name: example
fieldset:
classes: app-fieldset--custom-modifier
attributes:
data-attribute: value
data-second-attribute: second-value
legend:
text: What is your nationality?
hint:
text: If you have dual nationality, select all options that are relevant to you.
errorMessage:
text: Please select an option
items:
- value: british
text: British
- value: irish
text: Irish
- value: other
text: Citizen of another country

- name: with error message
data:
name: waste
Expand Down
2 changes: 1 addition & 1 deletion src/govuk/components/checkboxes/template.njk
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
{% if hasHint %}
{{ govukHint({
id: itemHintId,
classes: 'govuk-checkboxes__hint',
classes: 'govuk-checkboxes__hint' + (' ' + item.hint.classes if item.hint.classes),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking I wonder if we could avoid the extra whitespace by doing something like

'govuk-checkboxes__hint' + ((' ' + item.hint.classes) if item.hint.classes)

Hopefully this'd output the space only when there are classes.

Copy link
Contributor

@NickColley NickColley Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current approach doesnt add whitespace after all so works well.

attributes: item.hint.attributes,
html: item.hint.html,
text: item.hint.text
Expand Down
129 changes: 112 additions & 17 deletions src/govuk/components/checkboxes/template.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,12 @@ describe('Checkboxes', () => {

describe('when they include an error message', () => {
it('renders the error message', () => {
const $ = render('checkboxes', examples['with all fieldset attributes'])
const $ = render('checkboxes', {
name: 'example',
errorMessage: {
text: 'Please select an option'
}
})

expect(htmlWithClassName($, '.govuk-error-message')).toMatchSnapshot()
})
Expand Down Expand Up @@ -667,13 +672,50 @@ describe('Checkboxes', () => {

describe('when they include a hint', () => {
it('renders the hint', () => {
const $ = render('checkboxes', examples['with all fieldset attributes'])
const $ = render('checkboxes', {
name: 'example',
hint: {
text: 'If you have dual nationality, select all options that are relevant to you.'
},
items: [
{
value: 'british',
text: 'British',
hint: {
text: 'Hint for british option here'
}
},
{
value: 'irish',
text: 'Irish'
},
{
hint: {
text: 'Hint for other option here',
classes: 'app-checkboxes__hint-other',
attributes: {
'data-test-attribute': true
}
}
}
]
})

expect(htmlWithClassName($, '.govuk-hint')).toMatchSnapshot()
})

it('associates the fieldset as "described by" the hint', () => {
const $ = render('checkboxes', examples['with all fieldset attributes'])
const $ = render('checkboxes', {
name: 'example',
fieldset: {
legend: {
text: 'What is your nationality?'
}
},
hint: {
text: 'If you have dual nationality, select all options that are relevant to you.'
}
})

const $fieldset = $('.govuk-fieldset')
const $hint = $('.govuk-hint')
Expand All @@ -687,11 +729,19 @@ describe('Checkboxes', () => {

it('associates the fieldset as "described by" the hint and parent fieldset', () => {
const describedById = 'some-id'
const params = examples['with all fieldset attributes']

params.fieldset.describedBy = describedById

const $ = render('checkboxes', params)
const $ = render('checkboxes', {
name: 'example',
fieldset: {
describedBy: describedById,
legend: {
text: 'What is your nationality?'
}
},
hint: {
text: 'If you have dual nationality, select all options that are relevant to you.'
}
})
const $fieldset = $('.govuk-fieldset')
const $hint = $('.govuk-hint')

Expand All @@ -700,13 +750,25 @@ describe('Checkboxes', () => {
)

expect($fieldset.attr('aria-describedby')).toMatch(hintId)
delete params.fieldset.describedBy
})
})

describe('when they include both a hint and an error message', () => {
it('associates the fieldset as described by both the hint and the error message', () => {
const $ = render('checkboxes', examples['with all fieldset attributes'])
const $ = render('checkboxes', {
name: 'example',
errorMessage: {
text: 'Please select an option'
},
fieldset: {
legend: {
text: 'What is your nationality?'
}
},
hint: {
text: 'If you have dual nationality, select all options that are relevant to you.'
}
})

const $fieldset = $('.govuk-fieldset')
const errorMessageId = $('.govuk-error-message').attr('id')
Expand All @@ -722,11 +784,22 @@ describe('Checkboxes', () => {

it('associates the fieldset as described by the hint, error message and parent fieldset', () => {
const describedById = 'some-id'
const params = examples['with all fieldset attributes']

params.fieldset.describedBy = describedById

const $ = render('checkboxes', params)
const $ = render('checkboxes', {
name: 'example',
errorMessage: {
text: 'Please select an option'
},
fieldset: {
describedBy: describedById,
legend: {
text: 'What is your nationality?'
}
},
hint: {
text: 'If you have dual nationality, select all options that are relevant to you.'
}
})

const $fieldset = $('.govuk-fieldset')
const errorMessageId = $('.govuk-error-message').attr('id')
Expand All @@ -738,14 +811,18 @@ describe('Checkboxes', () => {

expect($fieldset.attr('aria-describedby'))
.toMatch(combinedIds)

delete params.fieldset.describedBy
})
})

describe('nested dependant components', () => {
it('have correct nesting order', () => {
const $ = render('checkboxes', examples['with all fieldset attributes'])
const $ = render('checkboxes', {
fieldset: {
legend: {
text: 'What is your nationality?'
}
}
})

const $component = $('.govuk-form-group > .govuk-fieldset > .govuk-checkboxes')
expect($component.length).toBeTruthy()
Expand Down Expand Up @@ -777,7 +854,25 @@ describe('Checkboxes', () => {
})

it('passes through fieldset params without breaking', () => {
const $ = render('checkboxes', examples['with all fieldset attributes'])
const $ = render('checkboxes', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"with all fieldset attributes" specified idPrefix but didn't test for it as its value in the example was the same as name - so you couldn't really tell if it was working or falling back to name.

It would be nice to have a test for idPrefix too, what do you think @NickColley? Or we can do it later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know idPrefix is only needed for items, which is why the snapshot has not changed, so I think that'd be something to do later.

name: 'example',
errorMessage: {
text: 'Please select an option'
},
fieldset: {
classes: 'app-fieldset--custom-modifier',
attributes: {
'data-attribute': 'value',
'data-second-attribute': 'second-value'
},
legend: {
text: 'What is your nationality?'
}
},
hint: {
text: 'If you have dual nationality, select all options that are relevant to you.'
}
})

expect(htmlWithClassName($, '.govuk-fieldset')).toMatchSnapshot()
})
Expand Down
11 changes: 11 additions & 0 deletions src/govuk/components/radios/__snapshots__/template.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ exports[`Radios when they include a hint renders the hint 1`] = `
>
This includes changing your last name or spelling your name differently.
</span>
<span id="example-item-hint"
class="govuk-hint govuk-radios__hint"
>
Hint for yes option here
</span>
<span id="example-2-item-hint"
class="govuk-hint govuk-radios__hint app-radios__hint-no"
data-test-attribute="true"
>
Hint for no option here
</span>
`;

exports[`Radios when they include an error message renders the error message 1`] = `
Expand Down
22 changes: 0 additions & 22 deletions src/govuk/components/radios/radios.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -258,28 +258,6 @@ examples:
text: No
checked: true

- name: with all fieldset attributes
data:
idPrefix: example
name: example
errorMessage:
text: Please select an option
fieldset:
classes: app-fieldset--custom-modifier
attributes:
data-attribute: value
data-second-attribute: second-value
legend:
text: Have you changed your name?
hint:
text: This includes changing your last name or spelling your name differently.
items:
- value: yes
text: Yes
- value: no
text: No
checked: true

- name: with very long option text
data:
name: waste
Expand Down
2 changes: 1 addition & 1 deletion src/govuk/components/radios/template.njk
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
{% if hasHint %}
{{ govukHint({
id: itemHintId,
classes: 'govuk-radios__hint',
classes: 'govuk-radios__hint' + (' ' + item.hint.classes if item.hint.classes),
attributes: item.hint.attributes,
html: item.hint.html,
text: item.hint.text
Expand Down
Loading