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 when the menu of the header is missing but a toggle is present #4206

Merged
merged 3 commits into from
Sep 22, 2023

Conversation

romaricpascal
Copy link
Member

@romaricpascal romaricpascal commented Sep 12, 2023

We need to keep the early return if the toggle is absent, as it's not mandatory for the header to have a navigation.

Closes #4130

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 12, 2023 11:18 Inactive
@github-actions
Copy link

github-actions bot commented Sep 12, 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.96 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 72.29 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.41 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.83 KiB

Modules

File Size
all.mjs 68.39 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.83 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 827ba90

@romaricpascal romaricpascal changed the title Throw an ElementError when the menu of the header is absent. Throw ElementError when the menu of the header is absent. Sep 12, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 12, 2023 16:57 Inactive
@romaricpascal romaricpascal linked an issue Sep 18, 2023 that may be closed by this pull request
1 task
@colinrotherham colinrotherham force-pushed the components-type-error branch 2 times, most recently from 090e1b6 to 655af8f Compare September 18, 2023 13:29
Base automatically changed from components-type-error to main September 18, 2023 14:27
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 18, 2023 14:57 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 19, 2023 09:39 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 19, 2023 13:25 Inactive
@colinrotherham colinrotherham force-pushed the header-missing-element-error branch 2 times, most recently from a166ded to 6bbf0a4 Compare September 19, 2023 13:37
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 21, 2023 15:23 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 22, 2023 10:14 Inactive
We need to keep the early return if the toggle is absent, as it's not mandatory for the header
to have a navigation.

It could be good useful to expand the validations a little there and verify:
- the presence of an `aria-controls` attribute on the `$menuButton`
- the case where a menu is present, but no toggle

This is outside of the scope of this PR, whose aim is to throw where v4 would return early.

Closes #4130
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 22, 2023 10:38 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 22, 2023 10:51 Inactive
this.$menu = $menu
this.$menuButton = $menuButton
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing this

Helps the compiler out a lot (narrows the types down) when adding to class fields after the checks

Strict mode will no longer warn that they might be null etc

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.

Approved, but can you fix my daft previous suggestion? Sorry

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 22, 2023 13:54 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 22, 2023 14:06 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 22, 2023 14:19 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 22, 2023 15:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4206 September 22, 2023 15:20 Inactive
@romaricpascal romaricpascal merged commit fff4f70 into main Sep 22, 2023
44 checks passed
@romaricpascal romaricpascal deleted the header-missing-element-error branch September 22, 2023 15:44
@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
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 Header initialisation if key HTML elements are missing
3 participants