-
Notifications
You must be signed in to change notification settings - Fork 20
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
Drop support for legacy browsers #4111
Conversation
6d66c39
to
a6d8252
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me overall, just a couple of things to look at, happy to discuss further 👍
app/views/govuk_publishing_components/components/_layout_for_public.html.erb
Show resolved
Hide resolved
app/views/govuk_publishing_components/components/_layout_for_public.html.erb
Outdated
Show resolved
Hide resolved
9c52f89
to
f53a936
Compare
f53a936
to
2c31d28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Ash, just a few more questions and suggestions.
strict mode
This change was originally included in the tech spike as modules will always run in strict mode, but the change is not included here, this may be fine as it appears standardx will capture strict mode JS errors when running js:lint
or bundle exec rake
locally or as part of the CI, but won't be captured in the jasmine tests, just want to check if this is something we need to explore further now or in the future?
Thinking about the scope of the PR to Drop support for legacy browsers, there could be a couple more changes that are worth including, non-blocking for this PR, just want to highlight them as it seems a good opportunity.
Remove ie.js
This file is served to Internet Explorer 8 and below in layout_for_public
This file bundles together html5shiv-printshiv.js
and json2
which can also be removed, json2.js example from the readme 😅
There is no reason to use this file unless fate compels you to support IE8, which is something that no one should ever have to do again.
Remove X-UA-Compatible meta tag
Removing the X-UA-Compatible meta tag is a recommendation in the release notes of v5.0.0 of govuk-frontend, I did not remove this when completing the upgrade work to V5 as we still provided some level of support for legacy browsers.
The change would involve removing the tag and related comment in layout_for_public, interestingly it does not appear to be used in the admin template.
Not essential, and can always come back to this as part of another cleanup PR when looking at removing/updating CSS/JS where we support legacy browsers, IE8 code comments example
Thanks @MartinJJones 👍
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this Ash! 👍
efb5b82
to
3cf6dc8
Compare
3cf6dc8
to
b106b37
Compare
b106b37
to
4ec6300
Compare
What / Why
type=module
to our JavaScript to drop support for legacy browsersjs-enabled
class so it only works in ES6 browserses6-components.js
again as a separate task unless you disagree with thistype=module
shouldn't have an affect on how our JS is executed, because we useDOMContentLoaded
, and according to MDN:In legacy browsers, this will stop the
static
layout_for_public
JS from executing. The application specific JS will stop running once the other frontend app PRs are merged. This means for a period of time betweenstatic
being deployed, and the frontend apps being deployed, the legacy browsers may have their application JS being executed, but the JS disabled styles from this gem. I'm not sure the full impact of this, and I'm not sure it will be that big of an issue. I did test this scenario infinder-frontend
, and thefinder-frontend
search had the non-JS styles from this gem, but it still had the client-side realtime search from it's own application.js. The page functionality still worked fine in that example, so I don't think there will be an issue. And I guess we can try and deploy the frontend apps quickly to avoid any issues once this change is deployed.I've put this as a breaking change in the changelog, since it stops the JS working in old browsers.
Visual Changes
Browsers that do not support ES6 will have the non
js-enabled
styles.