Update checkboxes and radio buttons to include item hint classes on i… #1648
Conversation
|
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. |
|
The changelog entry in this pull request will need updating as v3.4.0 has now been released. |
|
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 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
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), | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The current approach doesnt add whitespace after all so works well.
NickColley
force-pushed
the
bugfix/include-hint-classes-on-checkbox-item
branch
from
9351341
to
f27e0a8
Compare
| @@ -279,10 +279,21 @@ examples: | |||
| items: | |||
| - value: british | |||
| text: British | |||
| hint: | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
bugfix/include-hint-classes-on-checkbox-item
branch
from
b34e569
to
5188f0a
Compare
| @@ -777,7 +854,25 @@ describe('Checkboxes', () => { | |||
| }) | |||
|
|
|||
| it('passes through fieldset params without breaking', () => { | |||
| const $ = render('checkboxes', examples['with all fieldset attributes']) | |||
| const $ = render('checkboxes', { | |||
There was a problem hiding this comment.
"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.
There was a problem hiding this comment.
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 |
…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__hintorgovuk-radios__hint. This PR adds the hint.classes value to this so that we can add custom classes.