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

Throw ElementError (not found) if checkboxes or radios have no input items #4236

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

domoscargin
Copy link
Contributor

Closes #4131

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4236 September 20, 2023 13:03 Inactive
@github-actions
Copy link

github-actions bot commented Sep 20, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-4.6.0.min.css 118.07 KiB
dist/govuk-frontend-4.6.0.min.js 47.1 KiB
dist/govuk-frontend-ie8-4.6.0.min.css 79.27 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 76.42 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 71.79 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.84 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 37.16 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.83 KiB

Modules

File Size
all.mjs 67.89 KiB
components/accordion/accordion.mjs 21.9 KiB
components/button/button.mjs 4.71 KiB
components/character-count/character-count.mjs 21.64 KiB
components/checkboxes/checkboxes.mjs 5.69 KiB
components/error-summary/error-summary.mjs 6.01 KiB
components/exit-this-page/exit-this-page.mjs 16.6 KiB
components/header/header.mjs 3.34 KiB
components/notification-banner/notification-banner.mjs 4.54 KiB
components/radios/radios.mjs 4.68 KiB
components/skip-link/skip-link.mjs 3.73 KiB
components/tabs/tabs.mjs 9.07 KiB

View stats and visualisations on the review app


Action run for 48903c5

@domoscargin
Copy link
Contributor Author

Suspect we also want to throw an error here, for the case where the element with targetId is not found, but not for the case where the input has no data-aria-controls attribute.

// Skip checkboxes without data-aria-controls attributes, or where the
// target element does not exist.
if (!targetId || !document.getElementById(targetId)) {
return
}

@domoscargin domoscargin marked this pull request as draft September 20, 2023 14:23
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4236 September 20, 2023 14:37 Inactive
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Am I allowed to approve it yet??

@domoscargin
Copy link
Contributor Author

Am I allowed to approve it yet??

Haha, I'm just looking at the stuff in my comment - which wouldn't be caused by a user-created parameter, except I guess in the case where they're using HTML and make a mistake.

@domoscargin domoscargin marked this pull request as ready for review September 20, 2023 14:53
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4236 September 20, 2023 14:53 Inactive
@domoscargin
Copy link
Contributor Author

@colinrotherham Should hopefully be good for review now.

@@ -51,13 +51,21 @@ export class Checkboxes extends GOVUKFrontendComponent {

this.$inputs.forEach(($input) => {
const targetId = $input.getAttribute('data-aria-controls')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We create item and conditional IDs internally in the template:

{% set conditionalId = "conditional-" + id %}

However, this check is still useful for folks using HTML instead of the Nunjucks macros, as they'd be inputting their own data-aria-controls and ids.

if (!targetElement) {
throw new ElementError(targetElement, {
componentName: 'Checkboxes',
identifier: `.govuk-checkboxes__conditional #${targetId}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how this is clearer, but realised we only select by ID?

Suggested change
identifier: `.govuk-checkboxes__conditional #${targetId}`
identifier: `#${targetId}`

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to revert this for this PR, but something to discuss when we review the content - is targetId user-friendly enough when there may be some kind of issue with it? Ideally having the parent radios item info in there would be great, but realise I may be going too far.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did some digging, and we used to scope it to $module before 266f877

I suppose since then the target could genuinely be anywhere on the page?

E.g. Tick a box in a left column and a question is revealed on the right?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4236 September 20, 2023 16:05 Inactive
We handle these IDs internally, but in the case where somebody is coding using HTML instead of the Nunjucks macros, they still might get caught out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Throw errors during Radios and Checkboxes initialisation if key HTML elements are missing
3 participants