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

Error message review: Use plain English and a selector/variable name in backticks as an identifier #4299

Closed
1 task
Tracked by #3478
domoscargin opened this issue Oct 4, 2023 · 6 comments
Assignees
Labels
documentation User requests new documentation or improvements to existing documentation javascript
Milestone

Comments

@domoscargin
Copy link
Contributor

domoscargin commented Oct 4, 2023

What

Following on from this doc, we're aiming to format all our error messages with a mixture of plain English and selectors/variable names in backticks.

In general, this might look like:

<COMPONENT>: <PLAIN_ENGLISH_TARGET_NAME> (<CSS_SELECTOR>) <ERROR>

For example:

Checkboxes: Input elements (`input[type="checkbox"]`) not found

Header: Button attribute (`.govuk-js-header-toggle[aria-controls]`) is not of type string

Character count: Form field (`.govuk-js-character-count`) is not of type `HTMLTextareaElement` or `HTMLInputElement`

Error summary: Root element (`[data-module=govuk-accordion]`) not found

Tabs: Link elements (`a.govuk-tabs__tab`) not found

Why

We want users to be able to quickly identify exactly what's caused the error. Using plain English in addition to a specific selector should hopefully provide all the context the user needs.

Who needs to work on this

Developer, Content Designer/Technical Writer

Who needs to review this

Content Designer/Technical Writer, Developer

Done when

  • All identifiers match the above format
@domoscargin domoscargin added documentation User requests new documentation or improvements to existing documentation javascript labels Oct 4, 2023
@domoscargin domoscargin added this to the v5.0 milestone Oct 4, 2023
@colinrotherham
Copy link
Contributor

colinrotherham commented Oct 4, 2023

Looks great in plain English 🙌

Got a few little inconsistencies we could tweak?

For example, keeping the named element at the start:

- aria-controls attribute on menu toggle (`.govuk-js-header-toggle`) is not of type string
+ Menu toggle (`.govuk-js-header-toggle`) attribute (`aria-controls`) is not of type string

Or instead of names like "Menu toggle" we sometimes suffix "element", "form control" or "form field" too

Could we solely use the name?

- Checkbox form controls (`input[type="checkbox"]`) not found
+ Checkboxes (`input[type="checkbox"]`) not found

Or if we want a suffix, can we narrow them down?

- Checkbox form controls (`input[type="checkbox"]`) not found
+ Checkbox elements (`input[type="checkbox"]`) not found

Last of all, shall we swap all JavaScript names to CSS selectors?

- Root element (`$module`) not found
+ Root element (`[data-module=govuk-accordion]`) not found

@colinrotherham
Copy link
Contributor

Cheers for the update @domoscargin, I like it

Do we want to roll your "Input element" pattern further?

- Character count: Form field (`.govuk-js-character-count`) is not of type `HTMLTextareaElement` or `HTMLInputElement`
+ Character count: Textarea element (`.govuk-js-character-count`) is not of type `HTMLTextareaElement` or `HTMLInputElement`
- Tabs: Links (`a.govuk-tabs__tab`) not found
+ Tabs: Link elements (`a.govuk-tabs__tab`) not found

It's accidentally a nice way to differentiate between "element" and "attribute" selectors too:

- Header: Button (`.govuk-js-header-toggle`) attribute (`aria-controls`) is not of type string
+ Header: Button attribute (`.govuk-js-header-toggle[aria-controls]`) is not of type string

But appreciate that's an element selector so keeping them separate works for me too

@domoscargin
Copy link
Contributor Author

domoscargin commented Oct 4, 2023

Some points from discussions on Slack:

  • We should try to remove the semantics of how we think of an element internally and describe the actual element (for example, Header: Menu Toggle <SELECTOR> becomes Header: Button <SELECTOR>
  • We've stuck with a generic "form field" for the Character Count, since there's some ambiguity around whether it's a textarea or input in a given user's code.

@romaricpascal
Copy link
Member

@claireashworth In terms of review of the error messages, will it be fine reviewing the strings in the code directly (they'll all appear in our component's tests)? would it be preferable to extract them in a list (as a comment of the PR, in a doc or wherever)?

@romaricpascal
Copy link
Member

romaricpascal commented Oct 9, 2023

A couple of thoughts on some of the outstanding questions:

Last of all, shall we swap all JavaScript names to CSS selectors?

- Root element (`$module`) not found
+ Root element (`[data-module=govuk-accordion]`) not found

What do we gain by standardising to CSS selectors everywhere? They're a good option most of the time, but I'm worried it'll be too restrictive too early to always go for them, as sometimes other technical info might be more suitable.

In this example, specifically, the error will only occur if people run their own querySelector (or pick the element some other way) and pass the result to our component. That means we can't really say for sure that their selector was [data-module=govuk-accordion] (even though thanks to our templates and our docs snippets there's a good chance it will be). The name of the constructor's first parameter will be more accurate.

Similarly, if we open to other content than CSS variables, we could open up to snippets of HTML which may be more easily understandable when it comes to content missing on the page:

- Checkbox form controls (`input[type="checkbox"]`) not found
+ Checkboxes (`<input type="checkbox">`) not found

I'd personally prefer reducing consistency (still with a preference for CSS selectors, as it's what our components will have used most of the time, but opening up to variables, HTML or whatever else) if it helps us be a bit clearer in places. Won't die on that hill though, just wanted to list alternatives that could be used as identifier.

It's accidentally a nice way to differentiate between "element" and "attribute" selectors too:

- Header: Button (`.govuk-js-header-toggle`) attribute (`aria-controls`) is not of type string
+ Header: Button attribute (`.govuk-js-header-toggle[aria-controls]`) is not of type string

But appreciate that's an element selector so keeping them separate works for me too

I find the first phrasing more helpful as it limits the need for undestanding CSS selectors, but again more of a preference than a strong blocker if we prefer the second.

Finally there's this little snowflake which I think will need its own thing: Skip link: $module.hash is not of type string. I think we'll need to add a bit of extra pain English around it to make clearer what needs fixing as Root element (`$module`) attribute (`href`) has no URL fragment sounds more helpful than talking about $module.hash. This bit might affect more #4300, though.

@domoscargin
Copy link
Contributor Author

Going to close this in favour of #4072 , where I've added a comment on some decisions: #4072 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation User requests new documentation or improvements to existing documentation javascript
Projects
Development

No branches or pull requests

3 participants