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

Defer loading of badge_only.css? #9134

Closed
mgeier opened this issue Apr 22, 2022 · 10 comments
Closed

Defer loading of badge_only.css? #9134

mgeier opened this issue Apr 22, 2022 · 10 comments
Labels
Improvement Minor improvement to code

Comments

@mgeier
Copy link
Contributor

mgeier commented Apr 22, 2022

Details

The RTD theme is not used, therefore badge_only.css is loaded.

Expected Result

Faster page load?

Actual Result

I don't know if that makes a meaningful difference, but the file https://assets.readthedocs.org/static/css/badge_only.css is loaded right at the beginning of the page load, even though the HTML elements it applies to are inserted into the page much later.
This seems like a waste, doesn't it?

I've played around with https://pagespeed.web.dev/, which reports potential savings of 780 ms (maybe to be taken with a grain of salt, but anyway).

I'm not at all an expert at any of this, but I've read that the CSS can be deferred with something like this:

<link rel="preload" href="my_style.css" as="style" onload="this.onload=null;this.rel='stylesheet'">
<noscript><link rel="stylesheet" href="my_style.css"></noscript>

The <noscript> fallback is probably not even necessary, since the "badge" itself is loaded with JavaScript.

Has something like this been considered before?

@mgeier
Copy link
Contributor Author

mgeier commented Apr 22, 2022

For testing, I hacked together a version to try this: https://insipid-sphinx-theme.readthedocs.io/en/temp-defer-css/

TBH, I can't tell if that improves anything.

@humitos
Copy link
Member

humitos commented Apr 25, 2022

Hi! Thanks for reporting this. I'm not an expert in this field either so, I'm pinging @agjohnson and @davidfischer so they can chime in.

@mgeier
Copy link
Contributor Author

mgeier commented May 22, 2022

I tried this for my own custom CSS overrides (mgeier/insipid-sphinx-theme#89) and it seems to work fine on Firefox and Chromium.

Any comments from the experts?

@mgeier
Copy link
Contributor Author

mgeier commented Jun 4, 2022

This could/should of course also be applied to https://assets.readthedocs.org/static/css/readthedocs-doc-embed.css

@mgeier
Copy link
Contributor Author

mgeier commented Aug 5, 2022

It might be complicated to remove FontAwesome from badge_only.css (see #9297), but I think trying to defer loading it would already help creating a more responsive experience.

Is anything speaking against that?

@stsewd stsewd added the Improvement Minor improvement to code label Aug 18, 2022
@humitos
Copy link
Member

humitos commented Sep 15, 2023

This badge_only.css is added by our custom Sphinx extension at https://github.com/rtfd/readthedocs-sphinx-ext/blob/83cf6ecae26d787c65ab848a7414f2b30f8c7299/readthedocs_ext/readthedocs.py#L80-L95

As I mentioned in another issue (#8323), we are working on a complete re-design of the flyout as part of a new project called addons (see https://blog.readthedocs.com/addons-flyout-menu-beta/)

We are testing "removing all these old files on the fly when serving the page" and opt-in into the new addons. Would you like to test this behavior on your insipid-sphinx-theme project and share some feedback about this?

@humitos
Copy link
Member

humitos commented Nov 7, 2023

We are testing "removing all these old files on the fly when serving the page" and opt-in into the new addons. Would you like to test this behavior on your insipid-sphinx-theme project and share some feedback about this?

@mgeier this is already deployed. You can enable it from the new beta dashboard (see the comment on #8497 (comment)). Please, let me know if this works for you.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 11, 2024

Sorry for my late response!

I didn't activate the addons in my real project (because it breaks my custom flyout placement), but I created a dummy copy for experimentation: https://just-testing-the-insipid-sphinx-theme.readthedocs.io/.

I have run the pagespeed test again (https://pagespeed.web.dev/analysis/https-just-testing-the-insipid-sphinx-theme-readthedocs-io-en-latest-showcase-tables-html/q75ory8lmr?form_factor=mobile) and it doesn't complain about badge_only.css anymore, which I guess is good!

However, the CSS file still seems to be loaded the same way (e.g. https://just-testing-the-insipid-sphinx-theme.readthedocs.io/en/latest/index.html):

<link rel="stylesheet" type="text/css" href="/_/static/css/badge_only.css" />

@humitos
Copy link
Member

humitos commented Jan 15, 2024

I didn't activate the addons in my real project (because it breaks my custom flyout placement)

Would you help me to get some feedback about the integration I've been working on for the new addons at readthedocs/addons#64? That will help theme authors to integrate the Read the Docs data (flyout included) into the documentation in a nice and customized way. I created an example in the Read the Docs Theme at readthedocs/sphinx_rtd_theme#1526?

@humitos
Copy link
Member

humitos commented Jan 15, 2024

However, the CSS file still seems to be loaded the same way (e.g. just-testing-the-insipid-sphinx-theme.readthedocs.io/en/latest/index.html):

Gotcha! Thanks for testing this out. It seems that I missed this URL when removing old stuffs. I added this file to the list of removals and deployed the fix. I tested your dummy copy and it doesn't load the badge_only.css anymore there. I'm closing this issue, but feel free to re-open if you consider that's not solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
Status: Done
Archived in project
Development

No branches or pull requests

3 participants