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.

Sign up for GitHub

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

Jump to bottom

Edit content of error messages #4351

Merged
merged 7 commits into from
Oct 19, 2023
Merged

Edit content of error messages #4351

merged 7 commits into from
Oct 19, 2023

Conversation

domoscargin
Copy link
Contributor

@domoscargin domoscargin commented Oct 18, 2023 •
edited
Loading

Closes #4072

Mostly following this comment: #4072 (comment)

And keeping track of which messages I've reviewed here: https://docs.google.com/spreadsheets/d/1_B5b1XLRzJLaJiAKpcW_ZByzjrHOVACRhrL29VVB7Lo/edit#gid=0

Custom messages

Thanks @colinrotherham for helping me out with this.

I've overloaded ElementError so it can be passed a straight-up string to be used as a message, or a set of options to construct a standard message. This allows us to easily cover unique cases.

Accordion

This PR doesn't address Accordion messages, since that PR is still open and can be rebased on this one.

@domoscargin domoscargin added this to the v5.0 milestone Oct 18, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 18, 2023 15:31 Inactive
@github-actions
Copy link

github-actions bot commented Oct 18, 2023 •
edited
Loading

📋 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.58 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 70.95 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.06 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.3 KiB

Modules

File Size
all.mjs 67.27 KiB
components/accordion/accordion.mjs 20.15 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 5d85080

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 18, 2023 16:56 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 18, 2023 16:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 07:19 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 08:32 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 09:03 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 09:04 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 09:08 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 09:14 Inactive
domoscargin
domoscargin commented Oct 19, 2023
View reviewed changes
packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs
element: this.$module,
expectedType: 'string'
})
throw new ElementError(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an example of passing a message instead of options.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 10:33 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 11:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 11:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 12:31 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 12:38 Inactive
@domoscargin domoscargin marked this pull request as ready for review October 19, 2023 12:40
colinrotherham
colinrotherham reviewed Oct 19, 2023
View reviewed changes
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.

Looking good @domoscargin

My comments are mainly thoughts for discussion really, see what you think

packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs Show resolved Hide resolved
packages/govuk-frontend/src/govuk/components/character-count/character-count.mjs Outdated Show resolved Hide resolved
packages/govuk-frontend/src/govuk/components/checkboxes/checkboxes.mjs Outdated Show resolved Hide resolved
packages/govuk-frontend/src/govuk/components/checkboxes/checkboxes.mjs Outdated Show resolved Hide resolved
packages/govuk-frontend/src/govuk/components/header/header.mjs Outdated Show resolved Hide resolved
packages/govuk-frontend/src/govuk/components/header/header.mjs Outdated Show resolved Hide resolved
packages/govuk-frontend/src/govuk/components/skip-link/skip-link.mjs Outdated Show resolved Hide resolved
packages/govuk-frontend/src/govuk/components/tabs/tabs.mjs Outdated Show resolved Hide resolved
@domoscargin @colinrotherham
Overload ElementError to allow either a message or options
cf14409 
In cases where we want a very different error message, we can now just pass that directly.

Co-authored-by: Brett Kyle <brett.kyle@digital.cabinet-office.gov.uk>
Co-authored-by: Colin Rotherham <colin.rotherham@digital.cabinet-office.gov.uk>
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 14:06 Inactive
@domoscargin
Copy link
Contributor Author

Thanks @colinrotherham - agreed with everything and have pushed some changes up.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 14:08 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 14:27 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 14:31 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 14:59 Inactive
colinrotherham
colinrotherham approved these changes Oct 19, 2023
View reviewed changes
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.

Fab work @domoscargin

Let's bring @romaricpascal in for HTML identifier feedback too

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4351 October 19, 2023 15:10 Inactive
romaricpascal
romaricpascal approved these changes Oct 19, 2023
View reviewed changes
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Looking great! The messages read nicely and it's a nice way to deal with one-off messages for the ElementError 🙌🏻 ⛵

@domoscargin domoscargin merged commit db8f8e8 into main Oct 19, 2023
44 checks passed
@domoscargin domoscargin deleted the bk-error-content-review branch October 19, 2023 15:20
romaricpascal pushed a commit that referenced this pull request Oct 27, 2023
@domoscargin
Merge pull request #4351 from alphagov/bk-error-content-review
83e7919 
Edit content of error messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romaricpascal romaricpascal romaricpascal approved these changes

@colinrotherham colinrotherham colinrotherham approved these changes

Assignees
No one assigned
Labels
javascript
Projects
GOV.UK Design System cycle board
Archived in project
Milestone
v5.0
Development

Successfully merging this pull request may close these issues.

Review content of thrown errors
4 participants
@domoscargin @romaricpascal @colinrotherham @govuk-design-system-ci