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

Prevent duplicate checkbox aria-describedby #1265

Merged

Conversation

colinrotherham
Copy link
Contributor

I've spent a bit of time in the checkbox component recently (passing describedBy around) so I've fixed #1248 as it'll benefit our work as well.

@colinrotherham colinrotherham force-pushed the duplicate-single-checkbox-aria branch from bbfe950 to fe527c1 Compare April 1, 2019 15:55
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1265 April 2, 2019 14:41 Inactive
@dashouse dashouse added the awaiting triage Needs triaging by team label Apr 3, 2019
@aliuk2012 aliuk2012 self-requested a review April 3, 2019 08:56
@kellylee-gds kellylee-gds removed the awaiting triage Needs triaging by team label Apr 3, 2019
@kellylee-gds kellylee-gds added 🕔 hours A well understood issue which we expect to take less than a day to resolve. Priority: Low labels Apr 3, 2019
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 good to me, thanks @colinrotherham 🙌 Just left one comment. We'll also need a second reviewer from our team to take a look.

src/components/checkboxes/template.njk Outdated Show resolved Hide resolved
Copy link
Contributor

@aliuk2012 aliuk2012 left a comment

Choose a reason for hiding this comment

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

Hi @colinrotherham

Thanks for this contribution.

Only one minor thing to help improve readability but apart from that it looks really good.

👍

src/components/checkboxes/template.njk Outdated Show resolved Hide resolved
@colinrotherham
Copy link
Contributor Author

Thanks for the feedback. I've been in Nunjucks land a while now, will make it simpler!

@colinrotherham colinrotherham force-pushed the duplicate-single-checkbox-aria branch from fe527c1 to 60d6742 Compare April 5, 2019 13:02
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1265 April 5, 2019 13:03 Inactive
@colinrotherham colinrotherham force-pushed the duplicate-single-checkbox-aria branch from 60d6742 to 7c818bf Compare April 5, 2019 13:04
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1265 April 5, 2019 13:04 Inactive
@colinrotherham
Copy link
Contributor Author

@aliuk2012 I needed to support this new hybrid case (previously untested) so I've pushed a slightly revised version up: https://govuk-frontend-review-pr-1265.herokuapp.com/components/checkboxes/with-error-message-and-hints-on-items/preview

As mentioned on the diffs, it's to cater for:

  1. Shows error message, so aria-describedby on fieldset
  2. Hint on checkboxes, so aria-describedby on inputs
  3. Error message ID should not be in input aria-describedby

Example

@hannalaakso If you'd still prefer those Nunjucks if/else simplifying further let me know.

@colinrotherham colinrotherham force-pushed the duplicate-single-checkbox-aria branch from 7c818bf to c5e923c Compare April 5, 2019 13:14
@colinrotherham
Copy link
Contributor Author

Rebased with master

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.

Thanks Colin!

@hannalaakso hannalaakso merged commit f2f4aee into alphagov:master Apr 8, 2019
@colinrotherham colinrotherham deleted the duplicate-single-checkbox-aria branch April 8, 2019 13:38
@aliuk2012 aliuk2012 mentioned this pull request Apr 11, 2019
@aliuk2012 aliuk2012 added this to the Next milestone Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕔 hours A well understood issue which we expect to take less than a day to resolve.
Projects
Development

Successfully merging this pull request may close these issues.

Checkbox can end up with multiple aria-describedby attributes when used without a fieldset but with a hint
6 participants