Ability to add attributes to select option #977
Conversation
src/components/select/select.yaml
Outdated
| @@ -17,6 +17,12 @@ examples: | |||
| value: 3 | |||
| text: GOV.UK frontend option 3 | |||
| disabled: true | |||
| - | |||
There was a problem hiding this comment.
Generally we're trying to only add to these examples when there's a visual difference that we need to be able to eyeball in the review app – we can omit this because we've got tests that cover this functionality.
It'd be good if we could remove this addition, and revert the corresponding changes to the README.
(sorry that this isn't documented anywhere – we must make this clearer)
There was a problem hiding this comment.
This should now be reverted, thanks for the quick review 😀
| @@ -151,6 +151,40 @@ describe('Select', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('when they include option attributes', () => { | |||
| it('it renders the option attributes', () => { | |||
There was a problem hiding this comment.
Minor – duplicated 'it', don't need to include it as part of the test name.
There was a problem hiding this comment.
Removed the duplicate, thanks.
|
@malcolmhire sorry, one more thing – would you be able to add a entry for this to the changelog? |
|
Thanks Malcolm – unfortunately there's a conflict because the CHANGELOG has changed on master. Would you be able to rebase against master? (I am very happy to do this on your behalf if you'd prefer) |
|
@36degrees Yeah if you go ahead and rebase for me, I don't often use rebase. |
36degrees
force-pushed
the
feature/select-option-attributes
branch
from
fa26abf
to
17096ca
Compare
36degrees
force-pushed
the
feature/select-option-attributes
branch
from
17096ca
to
7e04169
Compare
There was a problem hiding this comment.
I've rebased this against master to resolve the conflict. I've also taken the liberty of squashing some of your commits together – I hope that's OK!
Happy this can now be merged once it's been reviewed by another member of the team – thanks again for all your work on this.
|
@NickColley would you mind re-reviewing? |
|
@36degrees yeah fine to squash my commits 🙂. No problem with helping out, it helps me out in my project once this is released. Also working a very similar feature for the date component, so should have that done sometime soon. 🙂 |
Within our service, we need the ability to add data attributes to a select option, I've used a very similar approach to how radio and checkboxes attributes were added by @colinrotherham to keep it simple.