-
Notifications
You must be signed in to change notification settings - Fork 327
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
Allow attributes on checkboxes/radios #942
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Colin,
Thanks for this!
The general approach seems great, as the addition to items mirrors the attributes approach we use for components.
There are some more house style changes required before we can merge this, let me know if you'd prefer me to do this for you.
This Pull Request will also need to be reviewed and agreed by another member of the team.
@@ -28,6 +28,13 @@ | |||
|
|||
([PR #N](https://github.com/alphagov/govuk-frontend/pull/N)) | |||
|
|||
- Allow attributes on checkboxes/radios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is adding a new feature this changelog entry should be under the feature title.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NickColley No problem, moved it to "New features"
@@ -43,6 +43,32 @@ examples: | |||
value: irish | |||
text: Irish | |||
|
|||
- name: with attributes on items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not documented, we are trying to only add .yaml examples where we cannot write a unit test for it.
In this case could you remove this example and test that this functionality works by adding a test in checkboxes/template.test.js
please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NickColley Ah that's fine, makes sense. Added two new tests!
@NickColley I've made those two review changes. Tests all passing 👍 |
Updated checkbox/radio readme to document new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work thanks Colin 👍
We'll need another review from another member of the team.
Looks good to me 👍 |
Unlike other form controls, the checkbox/radio templates only allow attributes on the wrapping container markup.
Services like ours add data attributes to some checkboxes:
Following the same pattern as the other input, textarea and select, details templates etc, the attributes are appended like this:
Hopefully this pull request will help other teams too 😊