Throw errors during accordion initialisation #4266

domoscargin · 2023-09-27T15:10:50Z

github-actions · 2023-09-27T15:15:23Z

📋 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	76.38 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs	71.71 KiB
packages/govuk-frontend/dist/govuk/all.mjs	3.8 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	37.66 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs	5.3 KiB

Modules

File	Size
all.mjs	68.04 KiB
components/accordion/accordion.mjs	20.92 KiB
components/button/button.mjs	4.16 KiB
components/character-count/character-count.mjs	20.41 KiB
components/checkboxes/checkboxes.mjs	5.3 KiB
components/error-summary/error-summary.mjs	5.46 KiB
components/exit-this-page/exit-this-page.mjs	15.47 KiB
components/header/header.mjs	3.3 KiB
components/notification-banner/notification-banner.mjs	4 KiB
components/radios/radios.mjs	4.3 KiB
components/skip-link/skip-link.mjs	3.29 KiB
components/tabs/tabs.mjs	9.05 KiB

View stats and visualisations on the review app

Action run for c632049

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs

domoscargin · 2023-10-04T15:07:19Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.puppeteer.test.js


-        describe('errors at instantiation', () => {
-          let examples
+      describe('errors at instantiation', () => {


Noticed we accidentally had our errors at instantiation wrapped within the localisation block.

domoscargin · 2023-10-04T15:10:08Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs

@@ -122,7 +122,7 @@ export class Accordion extends GOVUKFrontendComponent {
    if (!($module instanceof HTMLElement)) {
      throw new ElementError($module, {
        componentName: 'Accordion',
-        identifier: '$module'
+        identifier: 'Root element (`[data-module=govuk-accordion]`)'


I've tried to get all the identifiers looking as described in #4299

domoscargin · 2023-10-04T15:11:12Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs

@@ -140,7 +140,7 @@ export class Accordion extends GOVUKFrontendComponent {
    if (!$sections.length) {
      throw new ElementError(null, {
        componentName: 'Accordion',
-        identifier: `.${this.sectionClass}`
+        identifier: `Element (\`.${this.sectionClass}\`)`


Wrinkle here:

I've just gone with using Element any time the element in question is a div, but do we want to be specific? Or add a less generic qualifier?

For example, here would we prefer something like Section element? (Though not that, because <section> is a thing).

Happy to drop "element" and just use the plain English name?

Accordion: Module ([data-module=govuk-accordion])

Accordion: Section (.govuk-accordion__section)

Accordion: Section header (.govuk-accordion__section-header)

Accordion: Section heading (.govuk-accordion__section-heading)

Accordion: Section button (.govuk-accordion__section-button)

etc

The word "Section" then doesn't have to mean <section> element

domoscargin · 2023-10-19T16:39:34Z

Rebased - allows this PR to be simplified, since all the beforeInitialisation wrangling has already been merged.

domoscargin · 2023-10-19T16:42:23Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs

@@ -411,7 +411,22 @@ export class Accordion extends GOVUKFrontendComponent {
    const $button = $section.querySelector(`.${this.sectionButtonClass}`)
    const $content = $section.querySelector(`.${this.sectionContentClass}`)

-    if (!$showHideIcon || !$showHideText || !$button || !$content) {
+    if (!$button) {


Note this is our second check for $button, so I haven't written another test for it. I guess technically you could trigger this slightly differently from the other instance, but the end result is an error. This will be neater if we figure out some kind of caching solution.

domoscargin · 2023-10-19T16:46:17Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.puppeteer.test.js

+          ).rejects.toEqual({
+            name: 'ElementError',
+            message:
+              'Accordion: Sections (`.govuk-accordion__section`) not found'


I'm tempted to suggest we have a new kind of message in ElementError of this sort:

No sections (<IDENTIFIER>) were found

For when a group of things is empty.

But I wouldn't consider this blocking.

Yeah, it does sound better, indeed, when multiple things are missing. Agreed this wouldn't block the PR and can be dealt with separately (wouldn't even block the pre-release, as the content is not part of the public API).

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs

romaricpascal

Spotted two little updates we could make to avoid "button" issues mentioning a <span> element, but happy for them to happen later on 😊

romaricpascal · 2023-10-20T09:42:30Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs

+    if (!$span) {
+      throw new ElementError({
+        componentName: 'Accordion',
+        identifier: `Section button (\`<span class="${this.sectionButtonClass}">\`)`


nitpick Maybe a little pedantic, but it looked weird to have "button" followed by a span tag.

Suggested change

identifier: `Section button (\`<span class="${this.sectionButtonClass}">\`)`

identifier: `Section button placeholder (\`<span class="${this.sectionButtonClass}">\`)`

romaricpascal · 2023-10-20T09:44:34Z

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs

+    if (!$button) {
+      throw new ElementError({
+        componentName: 'Accordion',
+        identifier: `Section button (\`<span class="${this.sectionButtonClass}">\`)`


nitpick Technically at that stage, it's an actual button tag, so we could either:

Suggested change

identifier: `Section button (\`<span class="${this.sectionButtonClass}">\`)`

identifier: `Section button (\`<button class="${this.sectionButtonClass}">\`)`

Which would help people look for a button, though what they likely need to do for fixing is making sure the placeholder is in so we turn it into a button with:

Suggested change

identifier: `Section button (\`<span class="${this.sectionButtonClass}">\`)`

identifier: `Section button placeholder (\`<span class="${this.sectionButtonClass}">\`)`

Thanks for spotting this

It's a quirk of Accordion that's caught me out too, that this.sectionButtonClass matches different things

Should we instead return early for !$button here, knowing we've thrown already for the placeholder?

Good idea! At this stage, we're the ones that created the <button> so we can early return like in other listener for now 😊

Thanks for taking a look, folks. I've now made these tweaks in the latest commit.

colinrotherham · 2023-10-20T16:26:54Z

@domoscargin I've rebased this PR with #4329 and #4345

We've renamed renderAndInitialise() to render() as not all components have JavaScript (to initialise)

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 September 27, 2023 15:11 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 September 28, 2023 23:13 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 2, 2023 06:54 Inactive

colinrotherham force-pushed the bk-accordion-errors branch from 77531f9 to 9fc9607 Compare October 2, 2023 06:54

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 2, 2023 06:55 Inactive

domoscargin force-pushed the bk-accordion-errors branch from 9fc9607 to 7a8d3c2 Compare October 3, 2023 12:40

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 3, 2023 12:40 Inactive

domoscargin marked this pull request as ready for review October 3, 2023 12:57

colinrotherham reviewed Oct 4, 2023

View reviewed changes

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs Outdated Show resolved Hide resolved

colinrotherham reviewed Oct 4, 2023

View reviewed changes

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs Outdated Show resolved Hide resolved

colinrotherham reviewed Oct 4, 2023

View reviewed changes

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs Outdated Show resolved Hide resolved

domoscargin commented Oct 4, 2023

View reviewed changes

colinrotherham mentioned this pull request Oct 13, 2023

Support beforeInitialisation() options, multiple modules #4330

Merged

colinrotherham force-pushed the bk-accordion-errors branch from 17a0798 to 7bb2dc7 Compare October 13, 2023 13:47

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 13, 2023 13:47 Inactive

colinrotherham force-pushed the bk-accordion-errors branch from 7bb2dc7 to 4141439 Compare October 13, 2023 16:54

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 13, 2023 16:54 Inactive

colinrotherham changed the base branch from main to puppeteer-arguments October 16, 2023 08:14

Base automatically changed from puppeteer-arguments to main October 17, 2023 14:37

domoscargin mentioned this pull request Oct 19, 2023

Edit content of error messages #4351

Merged

domoscargin force-pushed the bk-accordion-errors branch from 4141439 to 53a2a6a Compare October 19, 2023 16:36

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 19, 2023 16:36 Inactive

domoscargin commented Oct 19, 2023

View reviewed changes

colinrotherham reviewed Oct 19, 2023

View reviewed changes

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs Outdated Show resolved Hide resolved

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 19, 2023 17:17 Inactive

domoscargin force-pushed the bk-accordion-errors branch from bbefc16 to df1a8cb Compare October 19, 2023 17:20

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 19, 2023 17:20 Inactive

romaricpascal reviewed Oct 20, 2023

View reviewed changes

domoscargin added 6 commits October 20, 2023 17:12

Move error tests into their own block

5bacb0c

Throw error if accordion has no sections

7ec67b2

Throw error when section headers are missing

b743d8e

Throw errors if section heading or button missing

ae204df

Throw error when section content is missing

3ef5092

Switch identifiers to HTML where possible

9b811a5

colinrotherham force-pushed the bk-accordion-errors branch from df1a8cb to 9b811a5 Compare October 20, 2023 16:23

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 20, 2023 16:24 Inactive

Only throw for section button placeholder, not button

c632049

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4266 October 23, 2023 10:15 Inactive

colinrotherham approved these changes Oct 23, 2023

View reviewed changes

domoscargin merged commit 4714006 into main Oct 23, 2023
43 checks passed

domoscargin deleted the bk-accordion-errors branch October 23, 2023 10:53

romaricpascal mentioned this pull request Dec 8, 2023

Release v5.0.0 #4540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw errors during accordion initialisation #4266

Throw errors during accordion initialisation #4266

domoscargin commented Sep 27, 2023

github-actions bot commented Sep 27, 2023 •

edited

Loading

domoscargin Oct 4, 2023

domoscargin Oct 4, 2023

domoscargin Oct 4, 2023

colinrotherham Oct 4, 2023

domoscargin commented Oct 19, 2023

domoscargin Oct 19, 2023

domoscargin Oct 19, 2023

romaricpascal Oct 19, 2023

romaricpascal left a comment

romaricpascal Oct 20, 2023

romaricpascal Oct 20, 2023

colinrotherham Oct 20, 2023

romaricpascal Oct 20, 2023

domoscargin Oct 23, 2023

colinrotherham commented Oct 20, 2023

	identifier: `Section button (\`<span class="${this.sectionButtonClass}">\`)`
	identifier: `Section button placeholder (\`<span class="${this.sectionButtonClass}">\`)`

	identifier: `Section button (\`<span class="${this.sectionButtonClass}">\`)`
	identifier: `Section button (\`<button class="${this.sectionButtonClass}">\`)`

Throw errors during accordion initialisation #4266

Throw errors during accordion initialisation #4266

Conversation

domoscargin commented Sep 27, 2023

github-actions bot commented Sep 27, 2023 • edited Loading

📋 Stats

File sizes

Modules

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoscargin commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham commented Oct 20, 2023

github-actions bot commented Sep 27, 2023 •

edited

Loading