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

Refactor HTML templates #467

Closed
wants to merge 17 commits into from
Closed

Refactor HTML templates #467

wants to merge 17 commits into from

Conversation

Blendify
Copy link
Member

@Blendify 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
Copy link
Member Author

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

@Blendify Blendify changed the title Clean up head block Refactor HTML templates Sep 28, 2017
@Blendify
Copy link
Member Author

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

@Blendify Blendify mentioned this pull request Sep 28, 2017
@ericholscher
Copy link
Member

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

@ericholscher
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

@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.

@astrojuanlu
Copy link
Contributor

astrojuanlu 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
Copy link
Member Author

I will have a look as soon as I can.

@oliver-sanders
Copy link

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
Copy link
Member Author

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 %}
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@Blendify
Copy link
Member Author

@Juanlu001 do you mind testing again?

@astrojuanlu
Copy link
Contributor

@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

@astrojuanlu
Copy link
Contributor

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

@jessetan
Copy link
Contributor

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 %}.
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jessetan
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

Blendify commented Dec 18, 2017 via email

@jessetan
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants