Move javascript back to head #545

Blendify · 2018-01-16T09:55:32Z

Fixes #328

Blendify · 2018-01-16T09:57:04Z

sphinx_rtd_theme/layout.html

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


This can probably stay in the footer?

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 was directed towards the script below the modernizr link

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.

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.

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

Blendify · 2018-01-16T10:03:32Z

sphinx_rtd_theme/layout.html

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


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

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

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

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

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.

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.

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?

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.

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

Blendify · 2018-01-30T02:19:39Z

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.

ericholscher · 2018-01-30T02:31:48Z

sphinx_rtd_theme/layout.html

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


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

jessetan

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 · 2018-01-30T19:49:36Z

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 · 2018-02-01T02:48:52Z

@ericholscher any word yet on performance differences?

ericholscher · 2018-02-05T09:03:22Z

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 · 2018-03-07T15:31:41Z

Any word on how this works on RTD?

astrojuanlu · 2018-09-23T12:58:11Z

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

astrojuanlu · 2018-11-03T10:56:55Z

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 readthedocs/readthedocs.org#4367, at least for me.

@Blendify

It was moved away from <head> to the bottom of the page in readthedocs#78. This is a compressed version of readthedocs#545 by @Blendify, which fixes readthedocs#328.

mgeier · 2018-11-04T10:57:17Z

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 · 2018-11-04T18:16:05Z

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

Blendify · 2018-11-04T18:16:24Z

Wrong button oops...

Blendify · 2018-11-04T18:19:01Z

@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 · 2018-11-05T15:45:59Z

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.

davidfischer · 2018-11-05T21:17:22Z

sphinx_rtd_theme/layout.html

+    {%- 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>


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

davidfischer · 2018-11-05T21:18:41Z

sphinx_rtd_theme/layout.html

    {% endif %}

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


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

Blendify · 2018-11-05T22:56:11Z

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

davidfischer · 2018-11-05T23:04:00Z

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

It don't think it matters.

Blendify · 2018-11-05T23:14:50Z

https://stackoverflow.com/questions/9271276/is-the-recommendation-to-include-css-before-javascript-invalid Says it does not matter except for mobile where the difference is very small and speculative parsing will probably come to mobile at some point anyway.

…

On Mon, Nov 5, 2018 at 6:04 PM David Fischer ***@***.***> wrote: Just noticed this but shouldn't we inject the javascript after css? It don't think it matters. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#545 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOeua-6pCfDIgpfUrRVFZQkbsqdPFEAcks5usMPkgaJpZM4RfgiR> .

-- Aaron Carlisle Project administrator for the Blender 3D Documentation Project Email: carlisle.b3d@gmail.com Website: https://blendify.github.io

jessetan · 2018-11-06T09:34:53Z

sphinx_rtd_theme/layout.html

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



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

jessetan · 2018-11-06T09:35:43Z

sphinx_rtd_theme/layout.html

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


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

jessetan · 2018-11-06T09:45:18Z

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.

astrojuanlu · 2018-12-05T06:10:48Z

🎉 thanks!

Move javascript back to head

433ce2f

Blendify requested review from ericholscher, agjohnson and jessetan January 16, 2018 09:55

Blendify commented Jan 16, 2018

View reviewed changes

Move jQuery function back to bottom

2a47e06

readthedocs deleted a comment from jessetan Jan 16, 2018

Blendify added 2 commits January 16, 2018 16:44

Do not change white space

0614961

Merge branch 'master' into java-head

cc0a99b

Blendify mentioned this pull request Jan 18, 2018

Content Security Policy support #414

Closed

Merge branch 'master' into java-head

9744003

Blendify mentioned this pull request Jan 25, 2018

0.3.0: Remaining todos #556

Closed

8 tasks

ericholscher reviewed Jan 30, 2018

View reviewed changes

Blendify added 2 commits January 29, 2018 21:44

Update comment + whitespace cleanup

6cd5a15

Merge branch 'master' into java-head

25b9abe

jessetan approved these changes Jan 30, 2018

View reviewed changes

Merge branch 'master' into java-head

c43d0b0

Merge branch 'master' into java-head

1f47e23

davidfischer mentioned this pull request Mar 28, 2018

gzip and nginx optimize module for static assets readthedocs/readthedocs.org#2955

Closed

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

stsewd mentioned this pull request May 17, 2018

Content Security Policy readthedocs/readthedocs.org#2793

Closed

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

astrojuanlu mentioned this pull request Sep 23, 2018

Plotly plots show locally but not on RTD readthedocs/readthedocs.org#4367

Closed

basnijholt mentioned this pull request Oct 22, 2018

prevent jQuery from being loaded twice readthedocs/readthedocs.org#4787

Closed

mgeier mentioned this pull request Nov 4, 2018

Move JavaScript back to <head> #696

Closed

Merge branch 'master' into java-head

0bff615

Blendify closed this Nov 4, 2018

Blendify reopened this Nov 4, 2018

ericholscher requested a review from a team November 5, 2018 15:43

davidfischer reviewed Nov 5, 2018

View reviewed changes

Address review points

bdef03a

jessetan reviewed Nov 6, 2018

View reviewed changes

Blendify added 2 commits December 4, 2018 20:13

Fix code comments

d9c1668

Merge branch 'master' into java-head

8e00c50

ericholscher approved these changes Dec 5, 2018

View reviewed changes

Blendify merged commit a42c4d7 into master Dec 5, 2018

Blendify deleted the java-head branch December 5, 2018 01:27

jessetan mentioned this pull request Oct 25, 2019

Defer loading JS #847

Closed

Move javascript back to head #545

Move javascript back to head #545

Conversation

Blendify commented Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blendify Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Blendify commented Jan 30, 2018

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessetan left a comment • edited Loading

Choose a reason for hiding this comment

Blendify commented Jan 30, 2018

Blendify commented Feb 1, 2018

ericholscher commented Feb 5, 2018

Blendify commented Mar 7, 2018

astrojuanlu commented Sep 23, 2018

astrojuanlu commented Nov 3, 2018 • edited Loading

mgeier commented Nov 4, 2018 • edited Loading

Blendify commented Nov 4, 2018

Blendify commented Nov 4, 2018

Blendify commented Nov 4, 2018

ericholscher commented Nov 5, 2018

davidfischer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blendify commented Nov 5, 2018

davidfischer commented Nov 5, 2018

Blendify commented Nov 5, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessetan commented Nov 6, 2018

astrojuanlu commented Dec 5, 2018 • edited Loading

Blendify commented Jan 16, 2018 •

edited

Loading

Blendify Jan 18, 2018 •

edited

Loading

jessetan left a comment •

edited

Loading

astrojuanlu commented Nov 3, 2018 •

edited

Loading

mgeier commented Nov 4, 2018 •

edited

Loading

astrojuanlu commented Dec 5, 2018 •

edited

Loading