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

fix: added polyfill for ES Modules and import map #504

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

Fredx87
Copy link
Contributor

@Fredx87 Fredx87 commented Aug 1, 2024

Description

This update makes the New Request Form compatible with old browsers that don't support ES modules or import maps.

Some old browsers don't support some features used in the new implementation of the request form. We officially support only the latest 2 versions of browsers on desktop and the latest version on mobile, but unfortunately there are still people using older versions, and so increasing the spectrum of supported browser can be a good thing. The drawback is that we are adding a 14kb script (gzipped) which is only needed for a very small number of browsers.

This PR adds https://github.com/guybedford/es-module-shims as an additional asset which is loaded in the document_head template. The official documentation suggests using a CDN, but I think it is better to include it directly in the theme as an asset to avoid external dependencies. I just copied the asset from the CDN and pasted it into the asset folder, I don't think it makes sense to install the library from NPM and add a build step for it since it is just a small script.

Screenshots

Tested on iPhone 7 with iOS 15 on the XCode simulator.

Before:
Screenshot 2024-08-01 at 15 12 17

After:
Screenshot 2024-08-01 at 15 11 00

Checklist

  • 📗 all commit messages follow the conventional commits standard
  • ⬅️ changes are compatible with RTL direction
  • ♿ Changes to the UI are tested for accessibility and compliant with WCAG 2.1.
  • 📝 changes are tested in Chrome, Firefox, Safari and Edge
  • 📱 changes are responsive and tested in mobile
  • 👍 PR is approved by @zendesk/vikings

@Fredx87 Fredx87 requested a review from a team as a code owner August 1, 2024 13:43
Copy link
Contributor

@BrunoBFerreira BrunoBFerreira left a comment

Choose a reason for hiding this comment

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

LGTM 👍 are there any tests that we need to add or update as well? Or do we usually just test these manually? I would imagine it is almost impossible to be automatic given that it is hardware dependent in this case.

@Fredx87
Copy link
Contributor Author

Fredx87 commented Aug 1, 2024

LGTM 👍 are there any tests that we need to add or update as well? Or do we usually just test these manually? I would imagine it is almost impossible to be automatic given that it is hardware dependent in this case.

I don't think we need any tests. We are just adding a polyfill, which provides a user implementation of a browser feature, and this should already be tested separately

Copy link
Contributor

@luis-almeida luis-almeida left a comment

Choose a reason for hiding this comment

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

The shim is not huge but does add some weight (40K but unzipped).
We're adding it in all pages but so far it's only relevant for the request form.
Maybe we could add it only in the request page for now (not a strong opinion) but check first if it's needed? e.g.:

if (!HTMLScriptElement.supports('importmap')) {
  // importmap not supported.
}

@Fredx87
Copy link
Contributor Author

Fredx87 commented Aug 1, 2024

The shim is not huge but does add some weight (40K but unzipped). We're adding it in all pages but so far it's only relevant for the request form. Maybe we could add it only in the request page for now (not a strong opinion) but check first if it's needed? e.g.:

if (!HTMLScriptElement.supports('importmap')) {
  // importmap not supported.
}

We actually need it on all pages, since we have a script in the document_head template as well. And in the future we will probably add more modules as well, so better to not restrict it to the request form.

The conditional loading is a good idea, I didn't try it because it is not mentioned in the official documentation, but it is mentioned here: guybedford/es-module-shims#371 (comment). I will give it a try and see if it works

This update makes the New Request Form compatible with old browsers that
don't support ES modules or import maps
@Fredx87 Fredx87 merged commit da1f942 into master Aug 2, 2024
5 checks passed
@Fredx87 Fredx87 deleted the gianluca/es-polyfill branch August 2, 2024 08:23
@zd-svc-github-copenhagen-theme
Copy link
Collaborator

🎉 This PR is included in version 4.0.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants