Preview v5: Follow GOV.UK Frontend coding standards #2961
Conversation
✅ You can preview this change here:
To edit notification comments on pull requests, go to your Netlify site configuration. |
colinrotherham
force-pushed
the
v5-classes
branch
from
90f9013
to
98abed8
Compare
colinrotherham
changed the title
colinrotherham
force-pushed
the
v5-classes
branch
from
98abed8
to
e5f45f6
Compare
colinrotherham
force-pushed
the
v5-types
branch
2 times, most recently
from
01a88fd
to
d1bdf84
Compare
colinrotherham
force-pushed
the
v5-classes
branch
from
e5f45f6
to
9157877
Compare
src/tsconfig.json
Outdated
There was a problem hiding this comment.
Everything that's in the src directory will get copied to deploy/public on build, which means that this file will get served at https://design-system.service.gov.uk/tsconfig.json.
Not a major issue but doesn't seem ideal either – any way we could move these into the base config but only apply them to src?
There was a problem hiding this comment.
Thanks for taking a look at this
Good point! I've filtered it out using Metalsmith's .ignore()
We could move it to the base but we need to scope only ./src to "lib": ["ESNext", "DOM"]
Will likely want the compiler to check ./lib and ./tasks in future, but without DOM libraries
| @@ -16,6 +16,6 @@ export default function loadAnalytics () { | |||
| j.async = true | |||
| j.src = `https://www.googletagmanager.com/gtm.js?id=${i}${dl}` | |||
| document.head.appendChild(j) | |||
| })(window, document, 'script', 'dataLayer', 'GTM-53XG2JT') | |||
| })(window, document, /** @type {const} */ ('script'), 'dataLayer', 'GTM-53XG2JT') | |||
There was a problem hiding this comment.
As this isn't really 'our' script, would it make more sense to turn off all type checking for the snippet?
There was a problem hiding this comment.
I'm happy with that. Added // @ts-nocheck like we did for polyfills in the past
|
|
||
| const $successNotification = $module.querySelector('.js-cookies-page-success') | ||
| if (!($successNotification instanceof HTMLElement)) { | ||
| return this |
There was a problem hiding this comment.
I guess we might follow up and change this to throw errors, but at the minute this has potential to trip us up by silently swallowing errors.
Given that $successNotification is only used within showSuccessNotification, could we move the lookup there, and return early if the banner isn't present?
That way at least the rest of the cookie page functionality will continue to work if the banner is missing for some reason.
There was a problem hiding this comment.
Good thinking
I've copied what we do elsewhere for optional selectors
const $successNotification = $module.querySelector('.js-cookies-page-success')
if ($successNotification instanceof HTMLElement) {
this.$successNotification = $successNotification
}☝️ Avoid appending to class property unless checks pass, but check again in showSuccessNotification()
colinrotherham
force-pushed
the
release/5.0
branch
from
f7a9918
to
06743de
Compare
…er of type 'string'"
colinrotherham
force-pushed
the
v5-types
branch
3 times, most recently
from
67e5a4e
to
399f2fb
Compare
Because we're not storing the object we create using `new`, we're getting a `no-new` failure from eslint.
colinrotherham
force-pushed
the
v5-types
branch
from
399f2fb
to
25c911c
Compare
This PR includes:
instanceofchecksinit()methods.parentElementversusparentNode.setAttribute()with boolean valuesI've also added a TypeScript compiler tsconfig.json config for code autocomplete etc
No compiler GitHub Actions checks are included, but follows what we did in: