Skip to content
This repository has been archived by the owner on Apr 19, 2021. It is now read-only.

self host fonts for better performance. #823

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nisarhassan12
Copy link
Contributor

@nisarhassan12
Copy link
Contributor Author

Before merging this it should be tested whether the loading performance is better or not i.e via a banchmark comparing this PRs preview with the production site.

@chrifro
Copy link
Contributor

chrifro commented Nov 16, 2020

according to Google PageSpeed there is a small speed improvement. Is this the impact that you expected @nisarhassan12 ?

Bildschirmfoto 2020-11-16 um 14 30 55

@nisarhassan12
Copy link
Contributor Author

@ChristinFrohne Yeah. I was expecting some improvement but not this much. On the deploy preview, the fonts don't seem to be loading at all maybe that is the case in the score improvement. It was not the case on the development preview. Do the fonts load for you on the deploy preview ? Thanks

@nisarhassan12
Copy link
Contributor Author

I have fixed the issue due to which the fonts were not loading. Someone should re-review this.

@gtsiolis
Copy link
Contributor

gtsiolis commented Dec 9, 2020

Looking at this now! 👀

@chrifro
Copy link
Contributor

chrifro commented Dec 9, 2020

Not sure how I can test it as the preview still uses the old embedded tweets and it's therefore a lot slower. Could you adjust the PR to the current state or do you have another idea how to test the performance?

@nisarhassan12
Copy link
Contributor Author

Thanks. @ChristinFrohne I have updated the Pr to the latest state. Please use Google Page Speed + test manually via disabling the cache both versions to see notable differences.

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @nisarhassan12! Changes LGTM.

However, have you tried using fontsource? The font used (Montserrat) is already available there and we could easily add this by running npm install fontsource-montserrat.

This is also one of the preferred ways for adding web fonts with Gatsby (see relevant guide) and woff+woff2 font files can provide quite sufficient browser support.

Let me know what you think. We can also add a follow-up issue to discuss this or create a new PR if we want to try this. 🏀

@nisarhassan12
Copy link
Contributor Author

Thanks, @gtsiolis.

However, have you tried using fontsource? The font used (Montserrat) is already available there and we could easily add this by running npm install fontsource-montserrat.

No, I haven't would definitely be something worth trying.

I'm a bit unsure regarding the self-hosted fonts or whether they make a positive impact on performance i.e see the below i.e in the before version the fonts seem to get loaded and rendered faster than the version with self-hosted self-hosted fonts which isn't What I was expecting. Can you also re-produce this? Thanks

Before After
Peek 2020-12-09 20-10 Peek 2020-12-09 20-05

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants