font example #215
font example #215
Conversation
|
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
|
|
|
at least for boilerplate it looks like there is no discernible performance impact.
|
davidnuescheler
requested review from
rofe and
kptdobe
and removed request for
rofe
scripts/scripts.js
Outdated
| @@ -105,6 +109,11 @@ async function loadLazy(doc) { | |||
| loadFooter(doc.querySelector('footer')); | |||
|
|
|||
| loadCSS(`${window.hlx.codeBasePath}/styles/lazy-styles.css`); | |||
|
|
|||
| loadCSS(`${window.hlx.codeBasePath}/styles/fonts.css`, () => { | |||
| if (!window.location.hostname.includes('localhost')) sessionStorage.setItem('fonts-loaded', 'true'); | |||
There was a problem hiding this comment.
Why sessionStorage and not localStorage ? If I frequently come back on the site on mobile (and close my browser app), I'll have to wait before the font is loaded while the font is cached already.
There was a problem hiding this comment.
Loading fonts.css a 2nd time on desktop... again, this probably works but not necessarily nice. We could use a global flag to track if we have loaded fonts.css or not. And also set the sessionStorage property in the eager phase too.
There was a problem hiding this comment.
localStorage is permanent, by default we cache for 2h for items coming from the code bus which seems much more of a session scope...
There was a problem hiding this comment.
i think loadCSS() would not add the fonts.css if it is already present, and if it did, the browser wouldn't load it twice either, so i think this is pretty safe.
There was a problem hiding this comment.
sessionStorage may throw an exception if quota has been exceeded or user disabled web storage. You may want to wrap in a try catch.
There was a problem hiding this comment.
good point. i have definitely never done that in the past, but i am not opposed to it.
scripts/scripts.js
Outdated
| if (window.innerWidth >= 900) loadCSS(`${window.hlx.codeBasePath}/styles/fonts.css`); | ||
| if (sessionStorage.getItem('fonts-loaded')) { | ||
| loadCSS(`${window.hlx.codeBasePath}/styles/fonts.css`); | ||
| } |
There was a problem hiding this comment.
This is calling loadCSS twice on desktop the 2nd time... browser is probably smart but could be optimised as:
| if (window.innerWidth >= 900) loadCSS(`${window.hlx.codeBasePath}/styles/fonts.css`); | |
| if (sessionStorage.getItem('fonts-loaded')) { | |
| loadCSS(`${window.hlx.codeBasePath}/styles/fonts.css`); | |
| } | |
| if (window.innerWidth >= 900 || sessionStorage.getItem('fonts-loaded')) { | |
| // load fonts.css immediately on desktop or if font has already been loaded | |
| loadCSS(`${window.hlx.codeBasePath}/styles/fonts.css`); | |
| } |
There was a problem hiding this comment.
i am not sure the extra code is needed given that loadCSS() already checks
| @@ -70,6 +70,10 @@ async function loadEager(doc) { | |||
| document.body.classList.add('appear'); | |||
| await waitForLCP(LCP_BLOCKS); | |||
There was a problem hiding this comment.
Why not calling the loadCSS before the waitForLCP ? The call is async and not blocker and downloading the css + the fonts could happen while we wait for the LCP to be completed ?
I am just thinking to the case where the LCP is a large text while will be impacted by the font loading.
There was a problem hiding this comment.
this is not about blocking but instead about bandwidth constraint... so if you have multiple requests go on in parallel, they will share the bandwidth, in which case the loading of the LCP would take 2x as long.
|
|
| @@ -70,6 +82,15 @@ async function loadEager(doc) { | |||
| document.body.classList.add('appear'); | |||
| await waitForLCP(LCP_BLOCKS); | |||
| } | |||
|
|
|||
| try { | |||
| /* if desktop (proxy for fast connection) or fonts already loaded, load fonts.css */ | |||
There was a problem hiding this comment.
fast connection
It's not widely supported yet but you could use navigator.connection.downlink to get the bandwidth estimate in megabits per second to know if the user has a fast connection or not https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/downlink
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
the vast majority of projects load custom web fonts. font loading while preserving a great user experience can be complex and often is dependent on the specific fonts that are being used.
this PR provides an example that should work for most projects, and combines and initial lazy load on mobile, to avoid impact to
LCPand loads fonts eager on subsequent pages on mobile. on desktop there is usually more bandwidth so loading fonts early can probably happen without materialLCPimpact (depending on the font).there are a number of open questions here:
(1) where should we put licensing terms for roboto
(2) the font-loading sequence should probably be related to bandwidth not screen size
(3) putting roboto into the repo, means that most project will have to remove those files and replace them with other self hosted font files
https://font-example--helix-project-boilerplate--adobe.hlx.live/
vs.
https://main--helix-project-boilerplate--adobe.hlx.live/