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.

Sign up for GitHub

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
Merged

Move javascript back to head #545

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
433ce2f
Move javascript back to head
Blendify Jan 16, 2018
2a47e06
Move jQuery function back to bottom
Blendify Jan 16, 2018
0614961
Do not change white space
Blendify Jan 16, 2018
cc0a99b
Merge branch 'master' into java-head
Blendify Jan 16, 2018
9744003
Merge branch 'master' into java-head
Blendify Jan 18, 2018
6cd5a15
Update comment + whitespace cleanup
Blendify Jan 30, 2018
25b9abe
Merge branch 'master' into java-head
Blendify Jan 30, 2018
c43d0b0
Merge branch 'master' into java-head
Blendify Feb 3, 2018
1f47e23
Merge branch 'master' into java-head
Blendify Mar 7, 2018
0bff615
Merge branch 'master' into java-head
Blendify Nov 4, 2018
bdef03a
Address review points
Blendify Nov 5, 2018
d9c1668
Fix code comments
Blendify Dec 5, 2018
8e00c50
Merge branch 'master' into java-head
Blendify Dec 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 37 additions & 44 deletions sphinx_rtd_theme/layout.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,47 @@
<link rel="canonical" href="{{ theme_canonical_url }}{{ pagename }}.html"/>
{% endif %}

{# CSS #}

{# OPENSEARCH #}
{# Javascript -- Keepin in head #}
Copy link
Member

Choose a reason for hiding this comment

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

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

{% if not embedded %}
{% if use_opensearch %}
<link rel="search" type="application/opensearchdescription+xml" title="{% trans docstitle=docstitle|e %}Search within {{ docstitle }}{% endtrans %}" href="{{ pathto('_static/opensearch.xml', 1) }}"/>
<script type="text/javascript">
var DOCUMENTATION_OPTIONS = {
URL_ROOT:'{{ url_root }}',
VERSION:'{{ release|e }}',
LANGUAGE:'{{ language }}',
COLLAPSE_INDEX:false,
FILE_SUFFIX:'{{ '' if no_search_suffix else file_suffix }}',
HAS_SOURCE: {{ has_source|lower }},
SOURCELINK_SUFFIX: '{{ sourcelink_suffix }}'
};
</script>
{%- for scriptfile in script_files %}
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Blendify Blendify Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

<script type="text/javascript" src="{{ pathto(scriptfile, 1) }}"></script>
{%- endfor %}

{# RTD hosts this file, so just load on non RTD builds #}
{% 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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<script type="text/javascript">
Copy link
Member Author

Choose a reason for hiding this comment

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

This can probably stay in the footer?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment was directed towards the script below the modernizr link

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

jQuery(function () {
{% if theme_sticky_navigation|tobool %}
SphinxRtdTheme.Navigation.enableSticky();
{% else %}
SphinxRtdTheme.Navigation.enable();
{% endif %}
});
</script>

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{# CSS #}
{# RTD hosts this file, so just load on non RTD builds #}
{% if not READTHEDOCS %}
<link rel="stylesheet" href="{{ pathto('_static/' + style, 1) }}" type="text/css" />
Expand Down Expand Up @@ -80,10 +111,6 @@
{%- endif %}
{%- endblock %}
{%- block extrahead %} {% endblock %}

{# Keep modernizr in head - http://modernizr.com/docs/#installing #}
<script src="{{ pathto('_static/js/modernizr.min.js', 1) }}"></script>

</head>

<body class="wy-body-for-nav">
Expand Down Expand Up @@ -185,40 +212,6 @@
</div>
{% include "versions.html" %}

{% if not embedded %}

<script type="text/javascript">
var DOCUMENTATION_OPTIONS = {
URL_ROOT:'{{ url_root }}',
VERSION:'{{ release|e }}',
LANGUAGE:'{{ language }}',
COLLAPSE_INDEX:false,
FILE_SUFFIX:'{{ '' if no_search_suffix else file_suffix }}',
HAS_SOURCE: {{ has_source|lower }},
SOURCELINK_SUFFIX: '{{ sourcelink_suffix }}'
};
</script>
{%- for scriptfile in script_files %}
<script type="text/javascript" src="{{ pathto(scriptfile, 1) }}"></script>
{%- endfor %}

{% endif %}

{# RTD hosts this file, so just load on non RTD builds #}
{% if not READTHEDOCS %}
<script type="text/javascript" src="{{ pathto('_static/js/theme.js', 1) }}"></script>
{% endif %}

<script type="text/javascript">
jQuery(function () {
{% if theme_sticky_navigation|tobool %}
SphinxRtdTheme.Navigation.enableSticky();
{% else %}
SphinxRtdTheme.Navigation.enable();
{% endif %}
});
</script>

{%- block footer %} {% endblock %}

</body>
Expand Down