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

Move javascript back to head #545

Merged
merged 13 commits into from Dec 5, 2018

Conversation

Projects
None yet
8 participants
@Blendify
Contributor

Blendify commented Jan 16, 2018

Fixes #328

@Blendify Blendify requested review from ericholscher, agjohnson and jessetan Jan 16, 2018

{% endif %}
<script src="{{ pathto('_static/js/modernizr.min.js', 1) }}"></script>
<script type="text/javascript">

This comment has been minimized.

@Blendify

Blendify Jan 16, 2018

Contributor

This can probably stay in the footer?

This comment has been minimized.

@jessetan

jessetan Jan 16, 2018

Contributor

According to Paul Irish, in the footer is the way to go for performance, unless you target IE6-8 with the html5shiv: Modernizr/Modernizr#878 (comment)
The current Modernizr in the theme does include the shiv, so moving it out of head might break IE6-8. Possible change would be to put the shiv in head and the rest of Modernizr at the end of the file. This relates to #525.

This comment has been minimized.

@Blendify

Blendify Jan 16, 2018

Contributor

This comment was directed towards the script below the modernizr link

This comment has been minimized.

@jessetan

jessetan Jan 16, 2018

Contributor

Whoops. Yes, that should be at the bottom. It is the code that starts the theme JS logic and should be run when the page (or at least the toctree) is complete.

This comment has been minimized.

@davidfischer

davidfischer Jan 16, 2018

Contributor

Putting JS in the footer is definitely better for performance in general but anything required for rendering should be in the header.

Not that we necessarily have to follow their lead, but Alabaster and the Spinx Basic theme (which Alabaster extends) both put JS in the head.

This comment has been minimized.

@davidfischer

davidfischer Jan 16, 2018

Contributor

I do think that attempting to have consistency across themes isn't a bad thing though.

SOURCELINK_SUFFIX: '{{ sourcelink_suffix }}'
};
</script>
{%- for scriptfile in script_files %}

This comment has been minimized.

@Blendify

Blendify Jan 16, 2018

Contributor

Add a comment saying not to use this? Use app.add_javascript instead.

This comment has been minimized.

@jessetan

jessetan Jan 16, 2018

Contributor

script_files is modified by search.html and the readthedocs-sphinx-ext, not sure what will happen if the theme removes this block.

This comment has been minimized.

@Blendify

Blendify Jan 16, 2018

Contributor

The extension will have to be updated. Currently css_files is deprecated in sphinx-doc/sphinx@83d3fd2 and I asked about script_files in sphinx-doc/sphinx#4439

This comment has been minimized.

@davidfischer

davidfischer Jan 16, 2018

Contributor

I believe @ericholscher is planning to remove many of the script_files from readthedocs-sphinx-ext. However, who knows if others are using it.

This comment has been minimized.

@ericholscher

ericholscher Jan 16, 2018

Member

I think we need to support people using them, since a lot of people use them. If we get to a point where Sphinx doesn't pass them anymore, then we can stop supporting them.

This comment has been minimized.

@Blendify

Blendify Jan 16, 2018

Contributor

There seems to be some confusing here. Note that this WILL be removed by sphinx 2.0.0 so we should add a comment in the html for people reading through the code so they know what the better option is.

This comment has been minimized.

@ericholscher

ericholscher Jan 16, 2018

Member

doesn't add_javascript just add it to script_files internally? So the public API is different, but for our theme it's the same API?

This comment has been minimized.

@Blendify

Blendify Jan 16, 2018

Contributor

Looking at the builder yes, I am still a little unsure of what direction sphinx will really go with this, so I'll ask. Since this does not affect this PR we can come back to this later.

This comment has been minimized.

@Blendify

Blendify Jan 18, 2018

Contributor

See this thread for why this is deprecated. So basically it can only be used for reading, not writing to unless through the sphinx app API. So we can keep script_files and css_files but we should add a comment and include release notes that extra_css_files` is deprecated so that we can avoid this type of user action. Again, we should include this in a separate commit/PR.

Thread https://groups.google.com/forum/#!topic/sphinx-dev/bvnF6Grw224

@rtfd rtfd deleted a comment from jessetan Jan 16, 2018

Blendify added some commits Jan 16, 2018

@Blendify Blendify referenced this pull request Jan 25, 2018

Closed

0.3.0: Remaining todos #556

4 of 8 tasks complete
@Blendify

This comment has been minimized.

Contributor

Blendify commented Jan 30, 2018

Poke. I say we forget about comments and just look at this from a functionality perspective.

@ericholscher

This looks good to me. I'd want to test it a little bit on various RTD builds to make sure it doesn't blow things up, but otherwise I'm +1 on the change.

{# CSS #}

{# OPENSEARCH #}
{# Javascript -- Keepin in head #}

This comment has been minimized.

@ericholscher

ericholscher Jan 30, 2018

Member

We can remove the second half of this comment. Feels silly.

Blendify added some commits Jan 30, 2018

@jessetan

jessetan approved these changes Jan 30, 2018 edited

LGTM assuming no breakages or slowdowns are experienced when testing with some larger projects on RTD.
Just throwing this out there, but how do you feel about making it a theme option to put the JS in the head? It is not needed, unless you're calling jQuery from inside rst like in #328. Putting JS in the head is risky for performance if large js libraries are included.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Jan 30, 2018

it should not change that much, we compress the js and we don't have that much js. I expect to only see a couple ms difference.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Feb 1, 2018

@ericholscher any word yet on performance differences?

@ericholscher

This comment has been minimized.

Member

ericholscher commented Feb 5, 2018

I'm not worried about performance, really. More about JS code people have written that depends on the current loading order, as well as the RTD specific code that we insert working with this loading order. I'll try and take a peek this week, but my week is pretty hectic.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Mar 7, 2018

Any word on how this works on RTD?

@agjohnson agjohnson modified the milestones: v0.3.0, v0.4.0 Mar 29, 2018

@stsewd stsewd referenced this pull request May 17, 2018

Open

Content Security Policy #2793

@agjohnson agjohnson removed this from the v0.4.0 milestone Jun 7, 2018

@Juanlu001

This comment has been minimized.

Juanlu001 commented Sep 23, 2018

I added some more info on why this would be useful here: rtfd/readthedocs.org#4367 (comment)

@Juanlu001

This comment has been minimized.

Juanlu001 commented Nov 3, 2018

I tried to rebase this locally but it's not obvious how to fix the conflicts without knowing the codebase. If @Blendify doesn't do it, I will use the old tip of the branch to see if it fixes rtfd/readthedocs.org#4367, at least for me.

mgeier added a commit to mgeier/sphinx_rtd_theme that referenced this pull request Nov 4, 2018

Move JavaScript back to <head>
It was moved away from <head> to the bottom of the page in rtfd#78.

This is a compressed version of rtfd#545 by @Blendify, which fixes rtfd#328.
@mgeier

This comment has been minimized.

mgeier commented Nov 4, 2018

I've also tried to rebase this, but it was indeed too hard for me, so I created a new PR moving those lines around: #696.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Nov 4, 2018

I rebased the patch so it should work fine. If you encounter problems let me know.

@Blendify Blendify closed this Nov 4, 2018

@Blendify Blendify reopened this Nov 4, 2018

@Blendify

This comment has been minimized.

Contributor

Blendify commented Nov 4, 2018

Wrong button oops...

@Blendify

This comment has been minimized.

Contributor

Blendify commented Nov 4, 2018

@ericholscher, @davidfischer, @jessetan, @agjohnson can you take a review of this? It seems many people need this so would be nice to put in the next release.

@ericholscher ericholscher requested a review from rtfd/core Nov 5, 2018

@ericholscher

This comment has been minimized.

Member

ericholscher commented Nov 5, 2018

I think it makes sense (especially since we're seeing users impacted by this issue). I'll let @davidfischer or @agjohnson chime in with any other considerations, but I'm 👍

@davidfischer

I had a few bits of feedback but this is close.

{%- if not READTHEDOCS %}
<script type="text/javascript" src="{{ pathto('_static/js/theme.js', 1) }}"></script>
{%- endif %}
<script src="{{ pathto('_static/js/modernizr.min.js', 1) }}"></script>

This comment has been minimized.

@davidfischer

davidfischer Nov 5, 2018

Contributor

You definitely want modernizr before loading 3rd party scripts ({%- for scriptfile in script_files %}) so that people can use modernizr features in their scripts.

{% endif %}

{% endif %}
{# RTD hosts this file, so just load on non RTD builds #}
{%- if not READTHEDOCS %}

This comment has been minimized.

@davidfischer

davidfischer Nov 5, 2018

Contributor

This if block should be removed. We are always including the theme.js now. See 41d251a#diff-1336f80431f476a232e3c48abb277c51.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Nov 5, 2018

Just noticed this but shouldn't we inject the javascript after css?

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Nov 5, 2018

Just noticed this but shouldn't we inject the javascript after css?

It don't think it matters.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Nov 5, 2018

title="{% trans docstitle=docstitle|e %}Search within {{ docstitle }}{% endtrans %}"
href="{{ pathto('_static/opensearch.xml', 1) }}"/>
{%- endif %}
{%- endif %}

This comment has been minimized.

@jessetan

jessetan Nov 6, 2018

Contributor

Nitpick: the {# CSS #} comment can be readded here

<link rel="search" type="application/opensearchdescription+xml"
title="{% trans docstitle=docstitle|e %}Search within {{ docstitle }}{% endtrans %}"
href="{{ pathto('_static/opensearch.xml', 1) }}"/>
{# Javascript #}

This comment has been minimized.

@jessetan

jessetan Nov 6, 2018

Contributor

Nitpick: Use {# JAVASCRIPT #} or {# SCRIPTS #} in all uppercase for consistency with other section comments

@jessetan

This comment has been minimized.

Contributor

jessetan commented Nov 6, 2018

Thanks for spending some more time on this @Blendify.
I haven't seen any feedback on compatibility other JS code folks may be using, does that mean it works fine or that we didn't test it?
Otherwise LGTM with nits.

Blendify added some commits Dec 5, 2018

@Blendify Blendify merged commit a42c4d7 into master Dec 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Blendify Blendify deleted the java-head branch Dec 5, 2018

@Juanlu001

This comment has been minimized.

Juanlu001 commented Dec 5, 2018

🎉 thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment