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

Add class .govuk-frontend-supported for ES modules support #3801

Merged
merged 3 commits into from
Jun 19, 2023

Conversation

colinrotherham
Copy link
Contributor

This PR updates the .js-enabled inline <script> to also append .govuk-frontend-supported

Only browsers with <script nomodule> support now run our JavaScript, which closes:

By additionally detecting 'noModule' in HTMLScriptElement.prototype (Safari 11+) we can "cut the mustard" and prevent early ES2015 class implementations from loading our components

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3801 June 12, 2023 17:08 Inactive
@colinrotherham colinrotherham linked an issue Jun 12, 2023 that may be closed by this pull request
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3801 June 12, 2023 17:12 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3801 June 12, 2023 17:18 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3801 June 13, 2023 07:18 Inactive
@colinrotherham
Copy link
Contributor Author

Do we need to follow our own advice and use the govuk-js-* prefix?

Noticed whilst updating test setup to include the new class:

document.body.classList.add('govuk-frontend-supported')
$textarea.classList.add('govuk-js-character-count')

Side-by-side it looks like a typo

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 neat! 🙌🏻 Just added a little comment to clarify comments in the Character count test (but pre-approving). I think this closes #3509 as well, as any further discussion on what the component does when it can't initialise can be handled as part of our work on the component's public API #3479.

Comment on lines 11 to 12
// Component checks that required elements are present
document.body.classList.add('govuk-frontend-supported')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Component checks that required elements are present
document.body.classList.add('govuk-frontend-supported')
// Component checks that GOV.UK Frontend is supported
document.body.classList.add('govuk-frontend-supported')
// Component checks that required elements are present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me. Were you happy that we don't comply with our .govuk-js-* prefix in #3801 (comment)?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, I think that's fine as the purpose of each class is different:

  • govuk-js-* for JS to grab specific HTML elements (and shouldn't be used for styling)
  • govuk-frontend-supported set by JS to hook styles, so shouldn't have the -js- bit (we happen to re-use it for checking if we're supported; we could have set a window.GOVUKFrontendSupported = true but that makes the snippet longer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brill, that's what I figured

Other services can freely use .govuk-js-* as advised by us

But we may see other "support classes" such as:

.govuk-frontend-supported
.govuk-prototype-kit-supported
.govuk-design-system-supported
.govuk-supported

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that'll be perfectly fine if these classes pop up as well, allowing services to have their own cut on top of ours (hopefully coordinating with ours, though 😆 ). ⛵

Additionally detects `'noModule' in HTMLScriptElement.prototype` (Safari 11+) rather than just baseline `<script type="module">` support
This closes the support gap for browsers without ES module support still loading JavaScript-only styles
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3801 June 19, 2023 16:46 Inactive
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jun 19, 2023

Thanks @romaricpascal

Let's merge this for now but aware we may still tighten the checks after:

For example we talked about raising the ES2015 bar to perhaps ES2018?

Reducing polyfills

For example this rough ES2018 feature detect for Promise.prototype.finally would avoid the need to ship polyfills to support browsers with potentially quite low visitor numbers:

'Promise' in window && 'finally' in window.Promise.prototype

Would give us loose assurance of the following:

  1. Array.prototype.includes() (ES2016)
  2. Object.values(), Object.entries() (ES2017)
  3. Promise async/await support (ES2017)
  4. Promise.prototype.finally support (ES2018)
  5. Spread operator and rest ... parameters (ES2018)

ES2015 to ES2018 support gap

Querying against promise-finally can give us some good figures:

Support gap between ES2015 and ES2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Implement new class for early styling
3 participants