Replace Skip link errors with ElementError
#4219
Conversation
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-4219
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 70265f1 |
colinrotherham
force-pushed
the
missing-to-element-error
branch
from
3d0cc44
to
f825dba
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-4219
| * @throws {MissingElementError} If the element with the specified ID is not found | ||
| * @throws {ElementError} when $module is not set or the wrong type | ||
| * @throws {ElementError} when $module.hash does not contain a hash | ||
| * @throws {ElementError} when the linked element is missing or the wrong type |
There was a problem hiding this comment.
@romaricpascal Just flagging that @domoscargin has documented @throw for JSDoc output
We only need them on public methods but I've updated them based on the test names
Presume this is something we'd want to roll out elsewhere after #4072
There was a problem hiding this comment.
Yeah, I think we'll want all our errors documented with @throws on the constructors once we've got the full extent of what's being thrown and the wordings.
| try { | ||
| const $linkedElement = this.getLinkedElement() | ||
| this.$linkedElement = $linkedElement | ||
| } catch (error) { |
There was a problem hiding this comment.
@domoscargin It got a bit tricky to handle all the error variations in a try/catch
Now that we have ElementError we can throw the same error type
For example, Skip link throws errors when its element property `$module.hash` does not contain a hash
colinrotherham
force-pushed
the
missing-to-element-error
branch
from
f825dba
to
70265f1
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-4219
There was a problem hiding this comment.
No blockers for me, though we may revisit messages once we have a final wording in #4072.
| * @throws {MissingElementError} If the element with the specified ID is not found | ||
| * @throws {ElementError} when $module is not set or the wrong type | ||
| * @throws {ElementError} when $module.hash does not contain a hash | ||
| * @throws {ElementError} when the linked element is missing or the wrong type |
There was a problem hiding this comment.
Yeah, I think we'll want all our errors documented with @throws on the constructors once we've got the full extent of what's being thrown and the wordings.
| throw new ElementError(this.$module, { | ||
| componentName: 'Skip link', | ||
| identifier: '$module.hash', | ||
| expectedType: 'string' | ||
| }) |
There was a problem hiding this comment.
remark Clever way to sort an "is not of the expected type" message. I think that's fine for this PR, but we may have to revisit once we settle on wording for the error messages depending on how things end up worded. Depending on our public API for errors, that might even be able to wait post v5, and be all internal anyways.
| getFragmentFromUrl(url) { | ||
| if (url.indexOf('#') === -1) { | ||
| return undefined | ||
| } | ||
|
|
||
| return this.$module.hash.split('#').pop() | ||
| return url.split('#').pop() |
There was a problem hiding this comment.
Good shout! 🙌🏻 Looks like a little function ripe for no longer living in these classes now they're both aligned (especially as it does not reference this).
| name: 'MissingElementError', | ||
| message: 'Skip link: $module "href" attribute does not contain a hash' | ||
| name: 'ElementError', | ||
| message: 'Skip link: $module.hash is not of type "string"' |
There was a problem hiding this comment.
Flagging for #4072 that we could harmonise the message for when an element is missing to " is missing" and use this here instead of discussing the type. Up for discussion when tackling that later issue 😊
Replace Skip link errors with `ElementError`
This PR replaces Skip link
$moduleand$module.hasherrors withElementErrorto close:MissingElementErrorwithElementError#4209