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

Assert Exit this page HTMLAnchorElement button and clearly guard all dynamic fields #4321

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Oct 10, 2023

This PR is a follow up to #4129 (see #4199) to fix the "link button" check:

- if (!($button instanceof HTMLElement)) {
+ if (!($button instanceof HTMLAnchorElement)) {

We should catch this early to ensure location.href changes work correctly

- window.location.href = this.$button.getAttribute('href')
+ window.location.href = this.$button.href

I've also added some extra guards for dynamic elements so it's clear which ones we chose to throw errors for

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4321 October 10, 2023 09:21 Inactive
@github-actions
Copy link

github-actions bot commented Oct 10, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-4.7.0.min.css 120.66 KiB
dist/govuk-frontend-4.7.0.min.js 51.56 KiB
dist/govuk-frontend-ie8-4.7.0.min.css 81.59 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 75.78 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 71.15 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.css 113.99 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 36.87 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.3 KiB

Modules

File Size
all.mjs 67.48 KiB
components/accordion/accordion.mjs 20.61 KiB
components/button/button.mjs 4.55 KiB
components/character-count/character-count.mjs 20.73 KiB
components/checkboxes/checkboxes.mjs 5.69 KiB
components/error-summary/error-summary.mjs 5.85 KiB
components/exit-this-page/exit-this-page.mjs 15.81 KiB
components/header/header.mjs 3.84 KiB
components/notification-banner/notification-banner.mjs 4.39 KiB
components/radios/radios.mjs 4.68 KiB
components/skip-link/skip-link.mjs 3.73 KiB
components/tabs/tabs.mjs 9.41 KiB

View stats and visualisations on the review app


Action run for 8d85f0a

@colinrotherham colinrotherham self-assigned this Oct 10, 2023
@colinrotherham colinrotherham added this to the v5.0 milestone Oct 11, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4321 October 12, 2023 12:30 Inactive
Copy link
Contributor

@domoscargin domoscargin 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, though I've had to get my head out of the Error messages space to allow it to accept early returns again 😆

Could we sneak in a coupla words to cover the guards in the PR title? Or give it a more general message about tightening up checks?

@colinrotherham colinrotherham changed the title Assert Exit this page HTMLAnchorElement button Assert Exit this page HTMLAnchorElement button and clearly show optional fields Oct 17, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4321 October 17, 2023 08:52 Inactive
@colinrotherham colinrotherham changed the title Assert Exit this page HTMLAnchorElement button and clearly show optional fields Assert Exit this page HTMLAnchorElement button and clearly guard all dynamic fields Oct 17, 2023
@colinrotherham
Copy link
Contributor Author

@domoscargin Thanks for reviewing, all updated and rebased

Copy link
Contributor

@domoscargin domoscargin left a comment

Choose a reason for hiding this comment

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

🚀

@colinrotherham colinrotherham merged commit e9d769e into main Oct 17, 2023
45 checks passed
@colinrotherham colinrotherham deleted the exit-this-page-link branch October 17, 2023 09:05
@colinrotherham colinrotherham removed their assignment Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants