-
Notifications
You must be signed in to change notification settings - Fork 232
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
Preview v5: Update to use <script type="module">
#2958
Conversation
✅ You can preview this change here:
To edit notification comments on pull requests, go to your Netlify site configuration. |
<script type="module">
<script type="module">
import ExamplePage from './components/example-page.mjs' | ||
|
||
// Initialise GOV.UK Frontend | ||
initAll({ |
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.
Example <iframe>
pages now use application-example.mjs
We adjust initAll()
from GOV.UK Frontend to turn off auto focus
@@ -11,6 +13,9 @@ import OptionsTable from './components/options-table.mjs' | |||
import Search from './components/search.mjs' | |||
import AppTabs from './components/tabs.mjs' | |||
|
|||
// Initialise GOV.UK Frontend | |||
initAll() |
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.
For application.mjs we can use the default initAll()
from GOV.UK Frontend
d434a21
to
a17c27f
Compare
<script type="module">
<script type="module">
I've not had a chance to do a full review but two things that stand out to me I wanted to note down before I forget:
|
Yeah, tricky isn't it Typically I'd suggest a server-side rendered version with It's where the GOV.UK Publishing ES6 to ES5 idea came from, so we could alternatively use Babel just on the Cookie banner component? Lots of risk Otherwise I'd assumed we were happy to lose the cookie banner by design 😬 I'd love to solve this though
I'm happy with this for the Design System website. We do add minor risk to First Contentful Paint (FCP) since the render-blocking CSS download may be delayed by a concurrent script download (albeit much lower priority) |
As discussed on Slack, we're happy that we don't need to split out the JavaScript for our analytics or cookie banner. This means that users visiting the Design System using a browser that does not support
Raw data
|
Let's leave this for now – maybe worth exploring in the future, if so we can look at running webpagetest and compare the two approaches? |
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.
I think the code changes generally look fine, but the experience for users on older browsers is suboptimal because we need to also update the Design System to use the new govuk-frontend-supported
class rather than js-enabled
, which requires a preview branch or pre-release.
Happy to approve this without those changes, but to makes sure we don't forget can you please create an issue for the govuk-frontend-supported
changes and assign it to the v5 milestone?
Thanks @36degrees, yeah it's awful isn't it I've followed the steps we took to upgrade GOV.UK Frontend in alphagov/govuk-frontend@7e1d0f4 Tests will be broken (no
Do you want an issue making for the other PRs? |
We need to ensure that only `initAll()` on example pages suppresses auto focus
a17c27f
to
ceef53a
Compare
Rebased with the preview release in: |
No description provided.