Skip to content
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

Fix font-display for Google Fonts #2588

Merged
merged 1 commit into from
Sep 20, 2019
Merged

Fix font-display for Google Fonts #2588

merged 1 commit into from
Sep 20, 2019

Conversation

csabapalfi
Copy link
Contributor

@csabapalfi csabapalfi commented Sep 18, 2019

Google Fonts shipped the display flag in May: https://addyosmani.com/blog/google-fonts-font-display/

This PR makes use of that new feature to add font-display: fallback in Google Fonts CSS via the display=fallback query param.

This will remove the following error under Diagnostics in Google PageSpeed Inisghts: https://developers.google.com/speed/pagespeed/insights/?url=https%3A%2F%2Fnodejs.org%2Fen%2F

Screenshot 2019-09-18 at 15 09 59

@XhmikosR
Copy link
Contributor

I thought they have a default which is swap, so this change seems redundant?

https://css-tricks.com/google-fonts-and-font-display/

Also, Source Sans Pro is used everywhere.

@csabapalfi
Copy link
Contributor Author

@csabapalfi
Copy link
Contributor Author

$ curl --silent "https://fonts.googleapis.com/css?family=Source+Sans+Pro:400,600" > default.css
$ curl --silent "https://fonts.googleapis.com/css?family=Source+Sans+Pro:400,600&display=swap" > swap.css
$ diff default.css swap.css 
4a5
>   font-display: swap;
10a12
>   font-display: swap;

@csabapalfi
Copy link
Contributor Author

Another thing that comes to mind is why we're loading this font on all pages? Isn't that only used on pages with code examples? Happy to raise another issue to start a discussion about that.

Removed this from the PR description as @XhmikosR confirmed the font is used everywhere.

@XhmikosR
Copy link
Contributor

Might be the default in the snippet they offer when copying. If so then this change makes sense.

Personally I tend to use fallback, but this depends on the project.

@csabapalfi
Copy link
Contributor Author

csabapalfi commented Sep 18, 2019

Happy to change to fallback. For anyone wondering what's the difference check out this great explainer: https://font-display.glitch.me/

@csabapalfi
Copy link
Contributor Author

Changed to fallback.

@XhmikosR
Copy link
Contributor

I don't mind either way personally, I was just saying what I use most of the time :)

Either way, it's an improvement for sure.

@Trott
Copy link
Member

Trott commented Sep 18, 2019

@nodejs/website

@SEWeiTung SEWeiTung merged commit faaaaf3 into nodejs:master Sep 20, 2019
@csabapalfi csabapalfi deleted the fix/font-display branch September 20, 2019 05:24
@XhmikosR
Copy link
Contributor

@csabapalfi: can you make the same change in core?

@Trott
Copy link
Member

Trott commented Sep 23, 2019

@csabapalfi: can you make the same change in core?

The relevant file is https://github.com/nodejs/node/blob/master/doc/template.html.

@XhmikosR
Copy link
Contributor

Yeah, I know, but I thought someone else could tackle this. :) No worries, I'll make a PR myself.

@csabapalfi
Copy link
Contributor Author

@XhmikosR you beat me to it! Great stuff! Thanks for pushing this ahead!

@Trott
Copy link
Member

Trott commented Sep 24, 2019

Yeah, I know, but I thought someone else could tackle this. :) No worries, I'll make a PR myself.

Oh, I was actually nothing that for @csabapalfi's benefit, but whatevs, as long as it gets updated, I don't really care who does it. :-D

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

Successfully merging this pull request may close these issues.

5 participants