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

Remove typekit fonts #3982

Merged

Conversation

davidfischer
Copy link
Contributor

This replaces the fonts loaded from Typekit with a native font stack.

The code fonts use the same stack as in readthedocs/sphinx_rtd_theme#612.
The regular text font uses the same font stack as GitHub.

Fixes #3939

- Replace fonts with a native font stack
- Fixes readthedocs#3939
@davidfischer davidfischer added Design Design or UX/UI related Improvement Minor improvement to code Needed: design decision A core team decision is required labels Apr 20, 2018
@davidfischer davidfischer requested a review from agjohnson April 20, 2018 18:14
@ericholscher
Copy link
Member

Seems like a solid step forward to me. I don't think we need fancy custom fonts enough to require a third party for this. 👍

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Ugh, yes i've wanted this gone for a long time now.

Any reason not to just use Lato and match the theme font for sans serif faces? I'd prefer that over using the GitHub font faces. We should also probably include our fonts via CSS while we drop typekit

@davidfischer
Copy link
Contributor Author

@agjohnson, we are discussing dropping Lato on the theme as well. Currently, it is ~1.5MB to download Lato on the theme (#3564). That would be reduced by only around half if we correctly gzipped them or served woff's. That's a lot of performance penalty for a font.

In general I'm +1 on a native font stack vs having a flash of unstyled text and longer load times but I don't feel that strongly.

@agjohnson
Copy link
Contributor

I'd be -1 on dropping Lato from our theme. It was originally used because it provided a better default experience for a wider range of non-CJK character sets. Its also an attractive type, which is baked into our theme experience -- i wouldn't want to leave display up to the OS here. I don't have a good solution for reducing load time here besides gzip compression though.

At very least, combining the asset source for docs and dashboard makes sense.

@davidfischer
Copy link
Contributor Author

While I believe that native OS fonts have gotten much better in the last few years with both Apple and Microsoft releasing high quality fonts for their platforms recently with great character support, I'm fine sticking with Lato and I'll rework this PR to use it. It is important to stay on brand. I'll see what can be done from the performance side. Most likely that will mean including the woff files in addition to the ttf.

At very least, combining the asset source for docs and dashboard makes sense.

I don't think I'm going to worry about this specifically in this PR as that will likely involve theme changes.

@davidfischer
Copy link
Contributor Author

I added Lato. Without subsetting the fonts (removing glyphs we don't need) the total for the regular, italic and bold versions of Lato are a little more than 1MB. Woff files are already compressed and so this can't be improved by gzipping.

Woff files are sufficiently supported I believe where we don't even need to deliver the TTFs at all: https://caniuse.com/#search=woff

@agjohnson
Copy link
Contributor

Aye, woff files should be enough by now. I'd say we can grep our logs to see ttf vs woff(2) usage of Lato currently, but our theme doesn't ship woff/woff2 :(

@agjohnson
Copy link
Contributor

Changes look fine. I'd say we should be installing through bower for more consistent upgrades, but I think a new theme will replace the need for this anyways.

@ericholscher
Copy link
Member

🎆

@ericholscher ericholscher merged commit 387a2f5 into readthedocs:master Apr 26, 2018
@davidfischer
Copy link
Contributor Author

Aye, woff files should be enough by now. I'd say we can grep our logs to see ttf vs woff(2) usage of Lato currently, but our theme doesn't ship woff/woff2 :(

This is probably a good change on sphinx_rtd_theme. We can get the improvements in performance without the nginx config changes around font gzipping.

Shipping woff only will mean formally dropping full support for IE8 and before (IE8 users would see a fallback font). There is some discussion of a formal browser support policy in readthedocs/sphinx_rtd_theme#525.

@agjohnson
Copy link
Contributor

I don't think we want to disallow ttfs, just serve woff with precedence. The addition would just be a woff/woff2 font option in our css font declaration.

@davidfischer
Copy link
Contributor Author

I'm fine with that. It will be around 700kb more per docs build since we will have woffs in addition to TTFs.

@davidfischer
Copy link
Contributor Author

It does look like older browsers also don't support the ttf though: https://caniuse.com/#feat=ttf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Design or UX/UI related Improvement Minor improvement to code Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants