Fix Bug 1445889, preload title and body fonts for improved performance #4709
Conversation
There was a problem hiding this comment.
Thanks @schalkneethling, this appears to work (although it is hard to say what would happen if it didn't). I have a couple of nits to address or ignore before you merge.
It's strange to me to ship a tag that FF doesn't recognize, but it looks like we did it for the interactive examples stuff, so it must be mostly working. We should find a way to preload those only on pages that include interactive examples...
jinja2/base.html
Outdated
| @@ -17,6 +17,10 @@ | |||
| "noindex, nofollow" | |||
| {%- endif -%} | |||
| > | |||
|
|
|||
| <!-- because this is non-blocking, preload as early as possible --> | |||
There was a problem hiding this comment.
This HTML comment seems more appropriate in preload.html, since it goes with the markup there. If you just want a comment for developers, I'd suggest a {# jinja comment #}.
jinja2/base.html
Outdated
| @@ -17,6 +17,10 @@ | |||
| "noindex, nofollow" | |||
| {%- endif -%} | |||
| > | |||
|
|
|||
| <!-- because this is non-blocking, preload as early as possible --> | |||
| {% include "includes/preload.html" %} | |||
There was a problem hiding this comment.
Why is this included as a separate file rather than inline?
There was a problem hiding this comment.
I am a bit of a stickler for this kinda thing. I get a little startled ;) when I open an HTML file and it is this huge wall of code. This is especially true for head sections.
This allows me to have this isolated, it does not add any performance issue, and is simple to update in future.
With all of that said, I am not resistant to change so, if you would prefer I inline this, I am more than happy to do so.
In this case browsers other than chrome will just ignore those. We have a large audience of Chrome users and so, it makes sense to roll this out as a progressive enhancement.
I noticed that these are added on pages where it is not required, as it is just a hint to the browser and not a preload of a bunch of content it is "ok". But I agree, I would want to fix this as soon as possible. Thanks for the review @jwhitlock |
Codecov Report
@@ Coverage Diff @@
## master #4709 +/- ##
==========================================
- Coverage 95.94% 95.47% -0.48%
==========================================
Files 270 262 -8
Lines 25352 23844 -1508
Branches 1743 1715 -28
==========================================
- Hits 24325 22766 -1559
- Misses 817 870 +53
+ Partials 210 208 -2
Continue to review full report at Codecov.
|
| @@ -0,0 +1,5 @@ | |||
| {# because this is non-blocking, preload as early as possible #} | |||
| <link rel="preload" href="{{ static('fonts/locales/ZillaSlab-Bold.woff') }}" as="font" type="font/woff" crossorigin /> | |||
There was a problem hiding this comment.
Why preloading the .woff file in addition to the .woff2 file ? Modern browsers will need only the .woff2 file as they support this format (and older browsers won't preload anything anyway)
No description provided.