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

Use native font stack / remove vendored open-sans + lato fonts #285

Merged
merged 13 commits into from
Mar 11, 2021

Conversation

jorisvandenbossche
Copy link
Member

Closes #247

@jorisvandenbossche
Copy link
Member Author

See https://pydata-sphinx-theme--285.org.readthedocs.build/en/285/ for a preview

cc @pradyunsg @choldgraf @hoetmaaiers
(Chris, note that this will change the look for jupyter-book users)

If we want this, I should still remove the vendored Lato fonts.

@choldgraf
Copy link
Collaborator

FYI here's the comparison of how it looks in the Jupyter Book theme:

CURRENT VERSION:
image

THIS PR:
image

FWIW I don't see a huge difference and in general I am a big fan of not shipping our own font stack

@pradyunsg
Copy link
Contributor

\o/

I like it.

Thanks for getting to this @jorisvandenbossche! Turns out, preparing for a dependency resolver, triaging a flurry of issues thanks to a beta, while also moving to a new country for a new job... means that you don't have much free time. I wish me-but-two-weeks-back was smarter and more pro-active about things. :)

@hoetmaaiers
Copy link
Collaborator

I am also fine with use a native font stack 👍

@jorisvandenbossche jorisvandenbossche changed the title Use native font stack Use native font stack / remove vendored open-sans + lato fonts Dec 29, 2020
@jorisvandenbossche jorisvandenbossche marked this pull request as ready for review December 29, 2020 09:02
@jorisvandenbossche
Copy link
Member Author

Updated to remove the vendored fonts.
We should probably give an example (and test) on how to switch back to Lato / open-sans yourself in your documentation.


One thing I noticed which I couldn't figure out yet is that the text doesn't seem to be centered anymore vertically.

For example, in the navbar, on master:

Screenshot from 2020-12-29 16-35-10

versus on this branch:

Screenshot from 2020-12-29 16-34-35

One can also notice it for the admonition titles (with this PR):

image

@jorisvandenbossche
Copy link
Member Author

The issue I mentioned above about alignment being a bit "off" seems to be a general Firefox+Linux issue (I see the same issue on GitHub as well ..), see eg chakra-ui/chakra-ui#2314 or primer/css#1209 for some discussion about it.

@jorisvandenbossche
Copy link
Member Author

One of the suggestions in that thread is to use "Helvetica Neue" instead of "Helvetica", so that Firefox doesn't use the wrong fallback for "Helvetica". On Firefox+Linux, that makes things look better indeed, and Chrome+Linux, that doesn't seem to make a difference (so that's good).

@choldgraf could you do a check that the demo docs with the latest commit also look OK for you?

@choldgraf
Copy link
Collaborator

They look good to me 👍

@choldgraf
Copy link
Collaborator

Is this ready to merge?

@bollwyvl
Copy link
Collaborator

So much beautiful red!

I dropped the quick note about quoting spaced font-family names, and potentially storing the fallback chain in its own variable, but otherwise LGTM.

docs/contributing.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@bollwyvl bollwyvl left a comment

Choose a reason for hiding this comment

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

❤️

@jorisvandenbossche
Copy link
Member Author

@bollwyvl thanks for the feedback!

There is one item remaining, which is the outdated documentation. Currently, we say that you can create a custom layout.html template file to override the fonts blocks:

{% extends "pydata_sphinx_theme/layout.html" %}

{% block fonts %}
    <!-- add `style` or `link` tags with your CSS `@font-face` declarations here -->
    <!-- ... and a `style` tag with setting `font-family` in `body` and `.header-style` -->
    <!-- ... and optionally preload the `woff2` for snappier page loads -->
    <!-- or add a `style` tag with a font fallback chain with good cross-platform coverage -->
    <style>
        body {
            font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, 'Open Sans', 'Helvetica Neue', sans-serif;
        }
        .header-style {
            font-family: Cambria, Cochin, Georgia, Times, 'Times New Roman', serif;
        }
    </style>
{% endblock %}

See https://pydata-sphinx-theme--285.org.readthedocs.build/en/285/user_guide/customizing.html#replacing-removing-fonts

Now, the <style> tag is not longer needed, as the above could be achieved with a custom.css file with

:root {
    --pst-font-family-base: Cambria, Cochin, Georgia, Times, 'Times New Roman', serif;
}

but you might still need to override the template if you want to include/download a custom font?

@bollwyvl
Copy link
Collaborator

LGTM! After this lands, we may be able to bump some performance thresholds, as loading n x [family, weight, style] files certainly wasn't free. This could be handled over #294 which adjusts some other audit-related things.

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Mar 10, 2021

but you might still need to override the template if you want to include/download a custom font?

For example if you want to add back Lato from a url? You could add

<link href='http://fonts.googleapis.com/css?family=Lato:400,700' rel='stylesheet' type='text/css'>

in the fonts block of the template?

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Mar 10, 2021

Although it seems that eg numpy does that inside the CSS: https://github.com/numpy/numpy/blob/2ea7ebdc4294868ec982ad6310d0daf6477fabfc/doc/source/_static/numpy.css#L1 (not sure which method is preferred, EDIT: although <link ..> is probably faster to load)

@bollwyvl
Copy link
Collaborator

ahh, yes, always docs-about-docs-about-docs...

but you might still need to override the template if you want to include/download a custom font?

Generally, fonts a site maintainer grabs off GF or whatever come as a bundle, and they'd want to download and unpack that into _static, then link to it, which could be done in custom.css with an @import. If they're doing npm... they've chosen their own path, and we can't anticipate what they'll do.

However, fonts (as we've learned) are special, and a maintainer may wish to preload them, etc. which cannot be specified in CSS directly. I don't see a big win to doing that on the config.py side of the house, so perhaps updating that section is all the guidance we need.

@bollwyvl
Copy link
Collaborator

<link ..> is probably faster to load)

Well, GF, or worse some of the "free fontz now" sites, are a rather large privacy leak. I don't think we'd want to recommend hot-linking to any resource, from a best practices point of view.

@jorisvandenbossche
Copy link
Member Author

OK, not really sure what I should put in our docs now .. ;)

Is it someting like:

  • Do you want to use a different font as default that is already available? -> Override the --pst-font-family variables in a custom.css
  • Do you want to use another custom font?
    • Include the source files in your repo (eg in _static)
    • Extend the layout.html template to load the font sources in the fonts block (using <link rel="stylesheet" href="{{ pathto('_static/..yourcustomfont..', 1) }}">)
    • And then specify the --pst-font-family variablesin a custom.css to use those fonts

?

@bollwyvl
Copy link
Collaborator

Started this sub-PR, but might be losing steam this evening 💤

jorisvandenbossche#1

Feel free to merge/copy/update as you see fit if i don't finish up!

@jorisvandenbossche
Copy link
Member Author

Thanks @bollwyvl ! I merged the PR to included it here.

Further, I did an attempt to update the customization section about how to change the default font (based on your updates and on what we discussed above). Could you take a look at that?

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Mar 11, 2021

For some reason, with one of the latest changes, the css is not loading anymore, see https://pydata-sphinx-theme--285.org.readthedocs.build/en/285/ (or not properly included in the build result, not sure, it gives a 404 not found). No idea what went wrong ..

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Mar 11, 2021

Update: forgot to git add the css/js ... 🤦
(but strange that our CI didn't catch that)

@bollwyvl
Copy link
Collaborator

I did see it thrashing a tad, but was only touching docs so I didn't do anything 😊 .

So question: while i was updating that, i talked about new icons... is there a compelling reason to not update to the latest fontawesome? the adobe icon was removed (in accordance with their brand stuff) and some unicode ranges got fixed. Also if a release is pending, the other JS deps? This could be a separate PR, of course, but the fonts at least seem germane to this topic.

@jorisvandenbossche
Copy link
Member Author

Let's keep updating other things for another PR, I would say (but don't hesitate to open one ;))

both better performance, and better script/glyph coverage than custom fonts,
and is recommended in most cases.

The default body and header fonts can be changed as follows:
Copy link
Member Author

Choose a reason for hiding this comment

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

@bollwyvl if you could take a quick look at this section (the text below here), that would be great

- If the font you want to specify in the section above is not generally available
by default, you will additionally need to ensure the font is loaded.
For example, you could download and vendor the font in the ``_static`` directory
of your sphinx site, and then update the base template to load the font resources:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Capitalize sphinx? I don't know.

{% endblock %}
<link rel="stylesheet" href="{{ pathto('_static/vendor/lato_latin-ext/1.44.1/index.css', 1) }}">

{% endblock %}
Copy link
Collaborator

@bollwyvl bollwyvl Mar 11, 2021

Choose a reason for hiding this comment

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

Perhaps concretely add:

To reduce the `Flash of Unstyled Content`, you may wish to explore various options for 
`preloading content <https://developer.mozilla.org/en-US/docs/Web/HTML/Preloading_content>`__, 
specifically the binary font files. This ensure the files will be loaded before waiting for the CSS to be
parsed, but should be used with care.

and then in the code block

   <link rel="preload" href="{{ pathto('_static/vendor/lato_latin-ext/1.44.1/files/lato-latin-ext-100-italic.woff2', 1) }}" as="font" type="font/woff2" crossorigin>

@bollwyvl
Copy link
Collaborator

The updates look good: I added a few small comments... the preload example can be dropped, but the MDN link is probably good to work in.

@choldgraf
Copy link
Collaborator

Once @bollwyvl is happy then I am happy to merge! I agree we should scope this PR cleanly and discuss other vendoring options as well 👍 let's get this merged! 🚀

@jorisvandenbossche
Copy link
Member Author

Thanks @bollwyvl, I added your suggested text

@jorisvandenbossche jorisvandenbossche merged commit 89d3885 into pydata:master Mar 11, 2021
@jorisvandenbossche jorisvandenbossche deleted the native-font-stack branch March 11, 2021 19:28
@bollwyvl
Copy link
Collaborator

bollwyvl commented Mar 11, 2021 via email

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.

Thoughts on dropping Lato, and using the system/native font stack instead?
5 participants