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

Refactor HTML templates #467

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Blendify
Contributor

Blendify commented Sep 19, 2017

This extends the default sphinx "basic" theme anything that is not there is added to extra head.

Fixes: #166:

Blendify added some commits Sep 19, 2017

@Blendify

This comment has been minimized.

Contributor

Blendify commented Sep 19, 2017

Note: this is untested code and I need to make sure it works still.

@Blendify Blendify requested review from ericholscher and agjohnson Sep 19, 2017

@Blendify Blendify changed the title from Clean up head block to Refactor HTML templates Sep 28, 2017

@Blendify

This comment has been minimized.

Contributor

Blendify commented Sep 28, 2017

@ericholscher @agjohnson can you have a look at this PR

@Blendify Blendify referenced this pull request Sep 28, 2017

Closed

Fix html head tag #447

@ericholscher

This comment has been minimized.

Member

ericholscher commented Sep 28, 2017

Is the idea that the deleted bits are already in the basic theme, and we inherit them?

@ericholscher

This comment has been minimized.

Member

ericholscher commented Sep 28, 2017

This seems like a really large change, tying ourselves to any additions in the Sphinx base theme. I imagine there could be many unindented outcomes.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Sep 28, 2017

Is the idea that the deleted bits are already in the basic theme, and we inherit them?

Yes

This seems like a really large change, tying ourselves to any additions in the Sphinx base theme. I imagine there could be many unindented outcomes.

Possible but doubtful, sphinx tries to keep this compatible since other theme do it to see: https://github.com/bitprophet/alabaster/blob/master/alabaster/layout.html

@Blendify

This comment has been minimized.

Contributor

Blendify commented Oct 25, 2017

@agjohnson @ericholscher what is the status here? I really think that this is the better way to go as the old way has already caused several errors that this could prevent.

@Juanlu001

This comment has been minimized.

Juanlu001 commented Nov 18, 2017

This pull request fixes the issues we're having with adding extra JavaScript libraries to display Plotly notebooks in Read the Docs, see spatialaudio/nbsphinx#128. There is only one issue with vertical space in the header:

master pr
screenshot-2017-11-18 poliastro - astrodynamics in python poliastro 0 7 dev0 documentation 5 screenshot-2017-11-18 poliastro - astrodynamics in python poliastro 0 7 dev0 documentation 4

(Edit: swap images)

The rest looks exactly the same and it would be great to have this merged 👍

@Blendify

This comment has been minimized.

Contributor

Blendify commented Nov 19, 2017

I will have a look as soon as I can.

@oliver-sanders

This comment has been minimized.

oliver-sanders commented Nov 21, 2017

js files in the script_files jinja2 list seem to be linked in twice, once in the header and once at the end of the document body.

Blendify added some commits Nov 21, 2017

@Blendify

This comment has been minimized.

Contributor

Blendify commented Nov 22, 2017

Note that to get exact behavior as before sphinx-doc/sphinx#4245 needs to get merged. But this is a minor thing and should not stop this PR.

{% if favicon %}
<link rel="shortcut icon" href="{{ pathto('_static/' + favicon, 1) }}"/>
{% endif %}
{# CANONICAL URL #}
{% if theme_canonical_url %}

This comment has been minimized.

@Blendify

Blendify Nov 22, 2017

Contributor

Note that this can be removed also if sphinx-doc/sphinx#4193 gets merged

Blendify added some commits Nov 22, 2017

@Blendify

This comment has been minimized.

Contributor

Blendify commented Nov 22, 2017

@Juanlu001 do you mind testing again?

@Juanlu001

This comment has been minimized.

Juanlu001 commented Nov 27, 2017

@Blendify the background gray stripe leveled up with the white body, but the whole block is still displaced. I attach current capture.

screenshot-2017-11-27 poliastro - astrodynamics in python poliastro 0 9 dev0 documentation

@Juanlu001

This comment has been minimized.

Juanlu001 commented Nov 27, 2017

You can test it by yourself by building these docs: https://github.com/poliastro/poliastro/tree/master/docs

@jessetan

This comment has been minimized.

Contributor

jessetan commented Dec 15, 2017

The gray stripe was two blocks of "related" stuff that was inherited from the basic theme, even more visible on less wide screens:
inherited blocks
Since this functionality is already covered by the sidebar and the prev/next buttons, I've removed them.

</p>
</div>
{%- if show_sphinx %}
{% trans %}Built with <a href="http://sphinx-doc.org/">Sphinx</a> using a <a href="https://github.com/snide/sphinx_rtd_theme">theme</a> provided by <a href="https://readthedocs.org">Read the Docs</a>{% endtrans %}.

This comment has been minimized.

@jessetan

jessetan Dec 15, 2017

Contributor

@Blendify why was this removed? When building the demo docs, this does not show up anymore, so I don't think it is inserted elsewhere.

This comment has been minimized.

@Blendify

Blendify Dec 15, 2017

Contributor

I thought it was inherited from the main layout file but I think that is not working. I can add it back.

@jessetan

This comment has been minimized.

Contributor

jessetan commented Dec 15, 2017

The search page has some extra text, currently it looks like this:
current search
Refactored, it uses the same content as the Sphinx theme, which has a search box on the results page:
search refactored
Given that the RTF theme always has the search box in the navbar, maybe the old way is better?

@Blendify

This comment has been minimized.

Contributor

Blendify commented Dec 18, 2017

@ericholscher @agjohnson what do you think about making the search page match one that looks like https://alabaster.readthedocs.io/en/latest/search.html I am thinking about making this a separate PR because it would mean that we get rid of the licence issue of having Sphinx code in our repository. See #283

@ericholscher

This comment has been minimized.

Member

ericholscher commented Dec 18, 2017

That would def. be a new PR. Our search stuff is complex, and I believe @agjohnson has plans to kill our dependence on the Sphinx search logic.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Dec 18, 2017

@jessetan

This comment has been minimized.

Contributor

jessetan commented Dec 19, 2017

I've made the change to use the same DOCTYPE as the theme does now, which should prevent rendering differences (if any) from Sphinx preferring XHTML.
We are still missing some IE-specific classes on <html>, but since there is no CSS specifically targeting IE8, I think its safe to leave out.

<!--[if IE 8]><html class="no-js lt-ie9" lang="{{ lang_attr }}" > <![endif]-->		
<!--[if gt IE 8]><!--> <html class="no-js" lang="{{ lang_attr }}" > <!--<![endif]-->
@ericholscher

This comment has been minimized.

Member

ericholscher commented Jan 10, 2018

Just as a note, I think this is a major change to the theme, and will require a lot of work in order to release properly. In particular, it ties us to the underlying Sphinx version in a lot of ways, and it will need to be tested across a bunch of different scenarios.

I'm imagining that this will be a 2.0 release of the theme. It will need to be tested across all supported Sphinx versions. It will also need to be tested across those Sphinx versions paired with RTD includes. Specifically, we have a lot of custom JS & CSS that could be changed by the underlying Sphinx HTML/CSS changes that come through, and we'd also be tying ourselves to supporting all new Sphinx features in the theme as soon as they are released in Sphinx itself.

As I said before without a lot of explanation, there is a huge amount of complexity that comes from this somewhat simple change. This PR should definitely reduce the amount going on, including not changing how we're doing search, so that we can properly test this and make sure what is coming across from Sphinx fully works in all the use cases that we need to support.

@jessetan

This comment has been minimized.

Contributor

jessetan commented Jan 12, 2018

This PR does fix some issues, in addition to delegating some stuff to the Sphinx theme.
Perhaps we can separate the fixes into other PRs (I think these mostly relate to where script tags are included), so this PR will not have functional effects, only remove things that we can inherit from the base Sphinx theme. This will give us some time to come up with a proper strategy to handle this PR, since it affects more than just the theme.

@Blendify

This comment has been minimized.

Contributor

Blendify commented Jan 15, 2018

Yes, I think this should be split up. It has become quite large and would be easier to split up. Also with Sphinx 2.0 coming in the next couple of years with some major changes likely. (we already know that css_files will be removed) maybe it would be better to drop version of sphinx lower than 2.0 at that time. We can save all the major (backward compatibility breaking) changes to the theme until then.

@Blendify Blendify closed this Jan 19, 2018

@Blendify Blendify deleted the refactor_html branch Jan 19, 2018

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