-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Performance: use inter static fonts #12501
Conversation
WalkthroughThe changes involve updating the font handling in a web application. The Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (8)
public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa0ZL7SUc.woff2
is excluded by:!**/*.woff2
public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa1ZL7.woff2
is excluded by:!**/*.woff2
public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa1pL7SUc.woff2
is excluded by:!**/*.woff2
public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa25L7SUc.woff2
is excluded by:!**/*.woff2
public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa2JL7SUc.woff2
is excluded by:!**/*.woff2
public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa2ZL7SUc.woff2
is excluded by:!**/*.woff2
public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa2pL7SUc.woff2
is excluded by:!**/*.woff2
src/fonts/Inter-VariableFont_slnt,wght.ttf
is excluded by:!**/*.ttf
Files selected for processing (4)
- src/lib/fonts.ts (1 hunks)
- src/pages/_app.tsx (2 hunks)
- src/pages/_document.tsx (1 hunks)
- src/styles/global.css (1 hunks)
Files skipped from review due to trivial changes (1)
- src/styles/global.css
Additional comments: 3
src/lib/fonts.ts (1)
- 9-9: The addition of
preload: false
to themono
font declaration aligns with the PR's objectives to optimize font loading. However, it's important to verify that this change does not adversely affect the loading times or performance of themono
font itself.src/pages/_document.tsx (1)
- 32-39: The preload link for the inter static web font is correctly implemented and should contribute to improved font loading times. Ensure that the specified font file path (
/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa1ZL7.woff2
) is correct and the font file is accessible.src/pages/_app.tsx (1)
- 42-42: The changes to the global CSS variables for fonts (
--font-inter
and--font-mono
) are correctly implemented and align with the PR's objectives. Ensure that these changes are consistently applied across the application to maintain uniform font rendering.Verification successful
The changes to the global CSS variables for fonts (
--font-inter
and--font-mono
) are correctly implemented insrc/pages/_app.tsx
and are also referenced insrc/@chakra-ui/foundations/typography.ts
. This confirms the consistent application of these font changes across the application, aligning with the PR's objectives. Ensure that these changes are consistently applied and tested across all relevant parts of the application to maintain uniform font rendering.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the consistent application of font changes across the application rg --type css --type html --type js --type tsx '--font-inter' --files-with-matches rg --type css --type html --type js --type tsx '--font-mono' --files-with-matchesLength of output: 993
Script:
#!/bin/bash # Correctly search for occurrences of '--font-inter' and '--font-mono' across the application rg --type css --type html --type js --type tsx '--font-inter' --files-with-matches rg --type css --type html --type js --type tsx '--font-mono' --files-with-matchesLength of output: 993
Script:
#!/bin/bash # Correctly search for occurrences of '--font-inter' and '--font-mono' across the application using the correct syntax rg --type css --type html --type js --type tsx -e '--font-inter' --files-with-matches rg --type css --type html --type js --type tsx -e '--font-mono' --files-with-matchesLength of output: 226
Script:
#!/bin/bash # Correctly search for occurrences of '--font-inter' and '--font-mono' across the application, excluding the unsupported 'tsx' file type rg --type css --type html --type js --type ts -e '--font-inter' --files-with-matches rg --type css --type html --type js --type ts -e '--font-mono' --files-with-matchesLength of output: 288
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.
Bummer that the variable font is so bulky, but agree that performance takes precedent here. My only request would be to use descriptive filenames instead of hashes so they're easier to reference/trace/understand. Otherwise looks good!
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.
Hm... is there any way we could clean up the typeface file names here so they're descriptive?
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.
Ah good call. Yeah, I can definitely do that.
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.
renamed: public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa2JL7SUc.woff2 -> public/fonts/inter/cyrillic-ext.woff2
renamed: public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa0ZL7SUc.woff2 -> public/fonts/inter/cyrillic.woff2
renamed: public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa2ZL7SUc.woff2 -> public/fonts/inter/greek-ext.woff2
renamed: public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa1pL7SUc.woff2 -> public/fonts/inter/greek.woff2
renamed: public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa25L7SUc.woff2 -> public/fonts/inter/latin-ext.woff2
renamed: public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa1ZL7.woff2 -> public/fonts/inter/latin.woff2
renamed: public/fonts/inter/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa2pL7SUc.woff2 -> public/fonts/inter/vietnamese.woff2
@pettinarip This change causes to execute more requests (and some fonts are failing), we would need to measure it to be sure its improving the performance, example of fonts loading on the homepage below |
You might have some of them cached or coming from a different source (extenstions perhaps?) because the ones that are failing are not being declared in this PR. There are others there in the screenshot that are also not part of this PR, they are probably coming from Netlify, for the Netlify UI elements of the preview deploy. Try to test this in incognito mode and lmk the results you get 🙏🏼 The other fonts, are ok, expected. The first 2 fonts are the most relevant fonts loaded here. The latin one is preloaded, the rest are loaded after the website is fully loaded and ready, so that is not going to affect the overall performance of the site. |
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.
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.
LGTM, thanks!
Description
Replaces the current variable inter font with static ones to improve loading times.
The variable font we are using is too heavy ~800kb. Loading the necessary static subset will improve the overall loading times.
Since
next/local
doesn't supportunicode-range
atm, we are loading and preloading the fonts manually.Summary by CodeRabbit