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 for missing elements during Character count instantiation #4261

Merged
merged 6 commits into from
Sep 27, 2023

Conversation

romaricpascal
Copy link
Member

Adds two errors thrown when:

  • The textarea is missing or not an HTMLTextareaElement or an HTMLInputElement
  • The textarea description is missing (preventing the component to replace its content with the live count)

Closes #4125

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4261 September 25, 2023 16:06 Inactive
@github-actions
Copy link

github-actions bot commented Sep 25, 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 77.38 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 72.69 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 120.45 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 37.65 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.83 KiB

Modules

File Size
all.mjs 68.79 KiB
components/accordion/accordion.mjs 21.91 KiB
components/button/button.mjs 4.7 KiB
components/character-count/character-count.mjs 22.02 KiB
components/checkboxes/checkboxes.mjs 5.68 KiB
components/error-summary/error-summary.mjs 6 KiB
components/exit-this-page/exit-this-page.mjs 16.61 KiB
components/header/header.mjs 3.83 KiB
components/notification-banner/notification-banner.mjs 4.54 KiB
components/radios/radios.mjs 4.67 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 808f7ec

@romaricpascal
Copy link
Member Author

romaricpascal commented Sep 25, 2023

Looks like we'll have to port the tests currently run in JSDom to Puppeteer. Even after adding an element for the textarea description, appending the markup to the document (so document.getElementById matches), cleaning it up between tests, we run into the lack of implementation of innerText from JSDom (hadn't seen this coming 😊 ) .

@colinrotherham
Copy link
Contributor

colinrotherham commented Sep 25, 2023

@romaricpascal Any quicker fixes to avoid innerText? So we don't lose code coverage

@romaricpascal
Copy link
Member Author

romaricpascal commented Sep 26, 2023

Digging a bit, we're using innerText to support IE8. I'll switch back to textContent and that should allow us to keep the tests in JSDom (I'll tidy the other occurences of innerText at the same time).

Now we throw if the textarea description is missing, that element needs to be in the DOM as well.
Our use of `document.getElementById` also meant that the elements need to be added to the `document`.
This meant that we also need some cleanup between tests as Jest JSDOM doesn't handle that between tests of a same file.
Finally, we also needed to use `textContent` rather than `innerText` as[JSDOM does not support the later](jsdom/jsdom#1245),
and we were only [using it for IE8](#2980), which we no longer need to support.
We are not quoting the identifier and having those around the types leads to unsightly quoting for the Character Count
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4261 September 26, 2023 11:29 Inactive
@romaricpascal
Copy link
Member Author

romaricpascal commented Sep 26, 2023

I think it's properly ready for review 😊

@domoscargin The removal of the quotes has a tiny bit of overlap with the tests you may be writing for the Tabs. It should be fairly easy to update with a search and replace for an instance of "(.*)" and of type "(.*)" once merged. Equally, we could be leave it until we've got all the errors and tidy up their wording, keeping the quotes for now. Not fussed either way, let me know what you'd prefer 😊

@romaricpascal romaricpascal marked this pull request as ready for review September 26, 2023 11:41
return this
throw new ElementError($textarea, {
componentName: 'Character count',
identifier: '.govuk-js-character-count',
Copy link
Contributor

Choose a reason for hiding this comment

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

With (potentially) multiple textarea components on the page, how much do we worry about scoping selectors against their module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Linking with #4263 (review)

For these "module child" selectors it could be brilliant to show a scope that the browser can inspect

Suggested change
identifier: '.govuk-js-character-count',
identifier: '.govuk-js-character-count',
scope: $module,

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 Yeah, that's probably the component most likely to be on a page multiple times. We recommend to start with a single question per page so that limits the risks. It doesn't completely avoids the possibility of multiple of those on the page (especially if we consider the radios/checkboxes potentially revealing fields). That said, there shouldn't be too many of them on a given page that it's tricky for developers to find back the one that's wonky (either by looking carefully at the page or breaking on error in the devtools). I would say it's not something we should not worry too hard about at the start. We can always dig deeper if we receive feedback.

If we have a simple solution, though, not against adding extra information, but it might be tricky to add that consistently. The element with data-module does not really have anything identifiable usually. Here, the $textarea could provide an id (but only if it is on the page so nothing if it doesn't exist).

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, there wasn't your second message when I started writing mine, sorry. How would users access the $module you propose passing as $scope (it's fine if that part still needs looking into)? Do browsers log extra fields from the errors? the cause maybe, in browsers that support it (that'd be neat)?

Copy link
Contributor

Choose a reason for hiding this comment

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

One to investigate. Would be amazing if you could hover over scope and it highlighted the element

Can leave them off for now if we don't need it

Copy link
Member Author

Choose a reason for hiding this comment

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

That'd speed debugging indeed, but let's see if people come back needing more pointers to where things happen as I think that'd need a good bit of investigation 😊 Had a quick glance at cause, Firefox logs it (only if the element itself, if an object, you get Object {...}, not expandable), not Chrome, nor Safari (at the time of this message). We could always override toString to log before the message gets computed for display in the console, but seems quite hacky anyway 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, not the clearest blurb. I'll (try to) clarify:

  • I'd prefer leaving it off until we have the need for it as it'd require more investigation to make it work
  • Got curious and gave a quick check at where Error's cause are logged (no luck there) and tried a quick hack by overriding toString to log before the error message gets computed (can log the element, but a bit hacky).

Copy link
Contributor

Choose a reason for hiding this comment

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

No don't worry, the confused face was wondering why cause wasn't logged

One for another day (and don't want to be Chrome specific) but some nice ideas in:
https://developer.chrome.com/docs/devtools/console/log/

).rejects.toEqual({
name: 'ElementError',
message:
'Character count: $module is not an instance of "HTMLElement"'
'Character count: .govuk-js-character-count is not of type HTMLTextareaElement or HTMLInputElement'
Copy link
Contributor

Choose a reason for hiding this comment

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

It works 🙌

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4261 September 26, 2023 14:06 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4261 September 26, 2023 16:36 Inactive
@colinrotherham colinrotherham merged commit 17ec1ef into main Sep 27, 2023
44 checks passed
@colinrotherham colinrotherham deleted the character-count-errors branch September 27, 2023 07:58
colinrotherham added a commit that referenced this pull request Sep 27, 2023
Throw `ElementError` for missing elements during Character count instantiation
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 Character Count initialisation if key HTML elements are missing
4 participants