-
Notifications
You must be signed in to change notification settings - Fork 0
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
feature: custom fonts with preloading #209
Conversation
Part of #87. # To do - [ ] add docs (assets/fonts/README.md?) - [ ] use swap with Fontaine?
Deploying head-start with Cloudflare Pages
|
src/assets/fonts.ts
Outdated
}, | ||
]; | ||
|
||
export const getFontCss = (font: Font) => { |
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.
Note: import raw (import archivo400css from '@fontsource/archivo/latin-400.css?raw';
) is not an option as it results in font urls starting with ./files/...
that are not resolved. So instead we generate the font declaration ourselves with the url that is resolved correctly in production.
Note 2: I've left out font-display: swap;
as we don't have a fallback yet that doesn't cause a layout shift. See to do on Fontaine and font swap.
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.
Using fontaine seems a bit involved. Rather keep this PR small and merge this first, to add font swap as perf enhancement in a separate PR.
/> | ||
)) | ||
} | ||
<style set:text={criticalCss}></style> |
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 dont think this serves any purpose since font files will only start to download when CSS is parsed and the browser knows wether there are characters on the screen that need a custom font. This is why font file downloads start late and we preload them with resource hints. Just a font face declaration will not cause the font to be downloaded.
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.
Or am I missing something?
If not, I think this PR can be "cleaned up" a lot. You could even remove the fonts.ts file all together since you only need the imports for the font files. So in perfhead something like this would be sufficient:
---
import archivo400url from '@fontsource/archivo/files/archivo-latin-400-normal.woff2?url';
import archivo600url from '@fontsource/archivo/files/archivo-latin-600-normal.woff2?url';
const fonts = [archivo400url, archivo600url]
---
<head>
{ fonts.map((font) => (
<link rel="preload" href={ font } as="font" type="font/woff2" crossorigin />
)) }
</head>
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.
You would still need to define the CSS declarations somewhere. It would make sense to me to do that in layouts/Default.astro
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.
Just checked again, and this does actually work. The reason being that both the critical CSS and the global core styles are inlined in the default layout. You can verify this on the the deploy preview. If you check the page source code you find both the font declarations (critical CSS) and the usage of the font family (might be easiest to search for --fontFamilyArchivo
). Alternatively, you can check this in the UI by using the Chrome Network DevTools and blocking the CSS file (_astro/index.Cs0OcUxF.css
) and verifying that the fonts are still loaded and applied.
Changes
Note: based on #208 so the
<PerfHead>
could easily be extended.Associated issue
Resolves #87
How to test
Checklist
I have made updated relevant documentation files (in project README, docs/, etc)created follow-up issue Document assets setup #210I have added a decision log entry if the change affects the architecture or changes a significant technology