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

Sphinx RTD responsive theme for documentation #1360

Merged
merged 15 commits into from Apr 15, 2018

Conversation

4 participants
@Bultako
Member

Bultako commented Apr 5, 2018

I have realized we do not need complex modifs to use a different template for the docs. I have used the Sphinx RTD responsive theme https://github.com/rtfd/sphinx_rtd_theme which is also the theme that is used for ctapipe https://cta-observatory.github.io/ctapipe/

You may have a look here
https://rawgit.com/Bultako/gammapy_rtd_docs/master/index.html

This PR is somehow related with Issue #1241
#1241 (comment)

@cdeil cdeil self-assigned this Apr 5, 2018

@cdeil cdeil added the docs label Apr 5, 2018

@cdeil cdeil added this to the 0.8 milestone Apr 5, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 5, 2018

@Bultako - Thanks!

I'm +1 to change the Gammapy Sphinx theme from the Astropy one to the RTD theme.

It is responsive, i.e. you can read nicely on phone or tablet, that is an objective, but not super important plus.

More important I think is what people prefer. @Bultako @registerrier @joleroi @adonath @leajouvin @robertazanin or anyone - which theme do you prefer?


Assuming we go ahead with this change, @Bultako - my suggestions would be:

I see this error in the web browser dev console:

GET https://rawgit.com/Bultako/gammapy_rtd_docs/master/_static/fonts/fontawesome-webfont.woff2?v=4.6.3 net::ERR_ABORTED

If it's easy to configure colors, could you try to use the two colors we use on http://gammapy.org/ ?
https://github.com/gammapy/gammapy-webpage/blob/66559b8fcb3d5c751af6ea5c809f14da1ea81379/style.css#L6

With the new theme, the main change is for navigation and the left sidebar.
There now it looks very crowded because we have such long headings like

Source detection tools (gammapy.detect)

These should be shortened somehow to make them each fit on one line, e.g. to just "Detection" or "Detection (gammapy.detect)" or just "gammapy.detect" or ... that is one thing that would probably need a little experimentation / discussion.

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 5, 2018

Another point is that with the new theme there is no table of contents / navigation available for the notebooks.
We have it here: http://docs.gammapy.org/0.7/notebooks/cta_data_analysis.html
But no table of content here: https://rawgit.com/Bultako/gammapy_rtd_docs/master/notebooks/cta_data_analysis.html
This might be difficult to do well, unless nbsphinx has a good solution to offer with the new theme?

@Bultako

This comment has been minimized.

Member

Bultako commented Apr 8, 2018

@cdeil
I think I have fixed all previous comments and issues in this thread.
Of course, there is still room for some more modifs in the CSS.
https://rawgit.com/Bultako/gammapy_rtd_docs/master/index.html

BTW
In case it was not obvious I'm also in favor of these sphinx_rtd_theme templated docs ;-)

@joleroi

This comment has been minimized.

Contributor

joleroi commented Apr 11, 2018

Intuitively I prefer the old theme, but I don't have a strong opinion. If the majority thinks the new theme is better I'm fine with that

@adonath

This comment has been minimized.

Member

adonath commented Apr 11, 2018

I prefer the new responsive theme. I think it looks better and more modern especially with the red and blue colors. It also works great on any smart device I tested (phone + tablet), much nicer than the old one. So +1 to use the new theme.

@joleroi

This comment has been minimized.

Contributor

joleroi commented Apr 11, 2018

I used it a bit and got used to it already. I just don't like new stuff in general 😉 So 👍 to use it

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 12, 2018

OK, so let's make this change.

Just for the record: I like almost everything a bit better about the new theme, although for the main change one has to admit that there are pros and cons: the navbar on the left always shows a full site table of contents in the new theme, and only a sub TOC for the given page in the old theme (see example for new, and for v0.7). In the new theme navigation to other places in the docs is easier, but the navbar is very long and it's a bit harder to find / read the TOC for the given page).

I'll try to build this locally now and either make small edits or leave inline comments as final review.

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 12, 2018

@Bultako - What about this?
https://travis-ci.org/gammapy/gammapy/jobs/363755664#L1619

Theme error:
sphinx_rtd_theme is no longer a hard dependency since version 1.4.0. Please install it manually.(pip install sphinx_rtd_theme)
The build_docs travis build FAILED because sphinx issued documentation warnings (scroll up to see the warnings).
Sphinx Documentation subprocess failed with return code 1

If there's no conda package for it, then probably sphinx_rtd_theme needs to be added here to make the docs test build work on travis-ci? (just a guess, I didn't look in detail)

- PIP_DEPENDENCIES='nbsphinx==0.2.17 sphinx-click'

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 12, 2018

@Bultako - when I try this PR locally, I see docs build errors that have been fixed in master recently. Can you please rebase this branch on master so that it's easier to try out locally?

@cdeil

Personally, I now find it difficult to find the page for a given Gammapy sub-package (e.g. gammapy.image) because the subpackage name no longer appears in the navbar:

screen shot 2018-04-12 at 10 13 12

@Bultako - OK to add it back?
It could be "image - short description" or "short description (image)" or just "gammapy.image". Not sure which is best, but I think the subpackage name in the outline is important so that people can find and navigate quickly.

Example: https://cta-observatory.github.io/ctapipe/

Instrument response function (IRF) functionality (`gammapy.irf`)
****************************************************************
**********************************
Instrument response function (IRF)

This comment has been minimized.

@cdeil

cdeil Apr 12, 2018

Member

Since we don't give abbreviations in the headings for other things, how about removing it here also and changing to this?

Instrument response functions
txt = re.sub(url_docs + '(.*?)html(\)|#)', r'..\1rst\2', txt, flags=re.M | re.I)
if folder == '_static/notebooks':
txt = re.sub(url_docs + '(.*?)html(\)|#)', r'..\/..\1html\2', txt, flags=re.M | re.I)
txt = re.sub(url_docs + '(.*?)html(\)|#)', r'..\/\1rst\2', txt, flags=re.M | re.I)

This comment has been minimized.

@cdeil

cdeil Apr 12, 2018

Member

This line contains a non-ascii character:
https://travis-ci.org/gammapy/gammapy/jobs/363755663#L1648

Please remove it.

I think maybe you introduced an apostrophe by copy & pasting?
https://unicodelookup.com/#re.sub(url_docs%20+%20'(.*?)html(\)|#)',%20r'..\/\1rst\2',%20txt,%20flags=re.M%20|%20re.I)/1

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 12, 2018

@Bultako - could you please check if there's an option to add line numbers to the code pages?
Example: https://rawgit.com/Bultako/gammapy_rtd_docs/master/_modules/gammapy/time/models.html#PhaseCurve

If yes, I'd be +1 to turn it on. If no, not important.

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 12, 2018

The color of links changes from red to violet after you have clicked them once:
Uploading Screen Shot 2018-04-12 at 10.24.21.png…

If possible, I would prefer to always keep the links red; IMO it's more of a distraction than a feature.
For docs, it's very common to re-read the same pages, or new pages, and what I read tomorrow might be different from yesterday. So this "record" of what was read by showing link colors isn't helpful IMO.

Also: I would be +1 to make the fonts a little larger. E.g. Github has 14 pt, the CTA website has 16 pt for normal text. I think you put 13 pt in the CSS. OK to increase e.g. to 14 pt?
I can then go through and just shorten e.g. headings to be more succinct and fit nicely in the table of contents as a follow-up commit here or in a separate PR.

@@ -1,11 +1,180 @@
/*
* Most code incorporated from bootstrap-astropy.css file in Astropy Project

This comment has been minimized.

@cdeil

cdeil Apr 12, 2018

Member

You are adding 185 lines of CSS in docs/_static/gammapy.css.
That's OK, but if it's possible to have less CSS with the same result (e.g. by inheriting CSS from the RTD theme), please make that change.
In Gammapy we're good at Python, but not CSS, so we should aim to have as little CSS as possible to be easy to maintain for the coming years.
Also a short mention in the dev docs how the docs theme works might be useful. E.g. it's not clear to me why you mention bootstrap-astropy.css here - do we still include their CSS or is this a modified copy of a lot of their CSS? Given that we use the RTD theme now, it's not obvious to me why we still need the astropy CSS!?

@cdeil

@Bultako Thanks for all your work! Feel free to merge whenever you're done here; just please check that the CI builds pass.

@Bultako

This comment has been minimized.

Member

Bultako commented Apr 14, 2018

@cdeil
Could you please have a look on how I did solve the unicode error in Travis with module sys ? Just in case you see a better way to do it.

It seems that what causes the glitch is something really hard to find and even harder to understand, which is line 28 in setup.cfg

captura de pantalla 2018-04-14 a las 13 42 57

I have been testing all this with a python 2.7 conda env locally..
If we put other thing than dev, things work.
Maybe devis a reserve keyword?

I have been forced to check the python version with sys.version_info[0] and encode/decode strings to/from 'utf-8' only for python<3


re: line numbers in code
I have not found a way to do it with automodapi, that is what we used to build the docs from the python code.


I have also realized that we lose the hide/show button for python prompts >>> that are at the top right corner of some code blocks.

This is an open issue of the RTD theme

I have tried other versions of jquery, as suggested in the thread, but that breaks other things in the theme.

p.s.
always a pain to work with sphinx, so sloooow :(

@Bultako

This comment has been minimized.

Member

Bultako commented Apr 14, 2018

@cdeil

Please forget my previous silly comment about the dev reserve keyword.
The glitch is caused because the regex only matches when we define the good url_docs, which was not the case since docs hosting moved from readthedocs.io

I have modified how to parse ipynb files with option encoding using the open/write functions of module io. I think it's more elegant.

If CI build pass and you're ok with that we can merge.

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 15, 2018

@Bultako - Thanks for all your work here! So is this ready to merge?

I would suggest to use
https://docs.python.org/3/library/pathlib.html#pathlib.Path.read_text
instead of io, because it's shorter and we should be as uniform as possible in Gammapy, so keep using Path and not start using io in some parts if it can be avoided.

What is copybutton.js? Did you copy it from somewhere or write it yourself? Hopefully it's just a copy and bundled and not code we have to maintain? If so, please leave a note where it came from somewhere where you think the next person that will hack on our docs might find it.

Otherwise, this looks great, 👍 to merge.

@Bultako

This comment has been minimized.

Member

Bultako commented Apr 15, 2018

👍 ready to merge.

implemented:

  • sphinx_rtd_theme as responsive template
  • full toc in left side bar navigation
  • gammapy.org css style
  • shorter consistent labels in menu
  • hide/show links for python prompts >>> in code blocks
  • gammapy logo favicon

not implemented:

  • line numbers in API code generated with automodapi.
txt = f.read()
with Path(filepath) as f:
txt = f.read_text(encoding="utf-8")

This comment has been minimized.

@cdeil

cdeil Apr 15, 2018

Member

Note that Path.read_text already has a with block that ensures the file handle is closed:
https://github.com/python/cpython/blob/1672c2fbae6128ee4717e08c4e65a0ab99a02a02/Lib/pathlib.py#L1190
So you can save one line:

txt = Path('example.txt').read_text()

I'll just make this tiny change now.

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 15, 2018

@Bultako - I changed the Path handling and some of the section headers a little bit in 1f8af6f .

Merging now.

This is great, thank you!

@cdeil

cdeil approved these changes Apr 15, 2018

@cdeil cdeil merged commit 4abe022 into gammapy:master Apr 15, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@cdeil cdeil added this to Done in Documentation via automation Apr 15, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 15, 2018

New theme is now online: http://docs.gammapy.org/dev/

Just for future reference: it looks like this at the moment:

screen shot 2018-04-15 at 22 06 17

@Bultako Bultako deleted the Bultako:fixdoc branch Apr 16, 2018

@Bultako

This comment has been minimized.

Member

Bultako commented Apr 16, 2018

@cdeil

It seems that nbsphinx behaves differently when you build the docs.
Have a look at the alert boxes here, how break lines have disappeared.

Maybe you have an editor that removes trailing spaces at the end of the lines. Lines 131 and 133 in utils/docs.py should have two trailing spaces that will be transformed to break lines by nbsphinx

captura de pantalla 2018-04-16 a las 16 43 43

It is important to have 3-lines boxes in the notebooks because there is a javascript that adds attribute download to the links, maybe necessary in some browsers to download linked files instead of rendering.

Moreover, I guess you are not using last version of sphinx_rtd_theme as declared in environment-dev.yml because I see sone errors of files missing in the browser inspection console :)

@cdeil

This comment has been minimized.

Member

cdeil commented Apr 17, 2018

@Bultako - I don't think I changed any trailing space.

Maybe I screwed up with my edits here?
1f8af6f#diff-0867a1cd7ff9664fe1d680927ea7ace2

@Bultako - Could you please revert or fix?

Another small improvement would be to make the favicon transparent, see:
screen shot 2018-04-17 at 13 43 51
Making it better visible by increasing the width of the lines would also be good, but is probably not easily possible.

@Bultako

This comment has been minimized.

Member

Bultako commented Apr 17, 2018

@cdeil
The problem is not due to the lines you point. I do have the good three-lines alert box in notebooks using the last commit of gammapy in GitHub. And I only can reproduce it if a remove the two-trailing spaces at the end of lines 131 and 133 in utils/docs.py

Maybe it's due to different versions of sphinx, nbsphinx or nbconvert?

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