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

Conversation

LBHELewis
Copy link
Contributor

…tem hint

We need to be able to add a custom class to all hints. Currently the checkbox and radio item loop outputs most hint properties, but not the classes, instead using only govuk-checkboxes__hint or govuk-radios__hint. This PR adds the hint.classes value to this so that we can add custom classes.

@NickColley
Copy link
Contributor

NickColley commented Nov 18, 2019

Thanks for spotting this and helping us fix it!

Can confirm that in our documentation we indicate you can add additional classes to the hints: https://design-system.service.gov.uk/components/radios/#options-inline-radios-example--hint

I don't think we add tests for components within components as we expect to be able to pass through all options but perhaps we should change this given we missed this option, will see what other reviewers think.

@NickColley NickColley added the awaiting triage Needs triaging by team label Nov 18, 2019
@36degrees
Copy link
Contributor

The changelog entry in this pull request will need updating as v3.4.0 has now been released.

@kellylee-gds kellylee-gds removed the awaiting triage Needs triaging by team label Nov 20, 2019
@NickColley
Copy link
Contributor

Hey again :)

I think we can get this merged if we update a test fixture and update the jest snapshots.

When we have nested components we render them and take a snapshot, you can see this with the test named 'renders the hint', in template.test.js.

This test should represent all the bits you could pass through to a component, just so we know it's doing what we expected.

So to test this functionality we can add hints to the with all fieldset attributes example in radios.yaml and then run the tests updating the snapshot:

node_modules/.bin/jest src/components/radios/template.test.js --updateSnapshot

You'll need to do the same for checkboxes component.

Let me know if you run into any problems.

@@ -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.

@NickColley NickColley force-pushed the bugfix/include-hint-classes-on-checkbox-item branch from 9351341 to f27e0a8 Compare December 9, 2019 13:42
@NickColley NickColley added this to the Next milestone Dec 9, 2019
@@ -279,10 +279,21 @@ examples:
items:
- value: british
text: British
hint:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be better served through adding new tests, as this fixture is meant to test checkboxes with 'all fieldset attributes' and these changes don't really fit that description.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've refactored the tests so the fixtures are specific for each test.

Instead of using a big single fixture for all tests, update the tests so they have the relevant fixtures for each test.
This will be used by the snapshot tests which will confirm that the hints render the correct information when it's given to them.
@NickColley NickColley force-pushed the bugfix/include-hint-classes-on-checkbox-item branch from b34e569 to 5188f0a Compare December 10, 2019 14:40
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Looks great, thanks everyone 🎉

The refactoring of tests feels like it could have been pulled into its own PR but I'm not too fussed.

@@ -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.

<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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

6 participants