-
Notifications
You must be signed in to change notification settings - Fork 588
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
New theme for documentation #3125
Conversation
👋 @alexisthual Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #3125 +/- ##
=======================================
Coverage 90.62% 90.62%
=======================================
Files 126 126
Lines 14898 14898
Branches 3060 3060
=======================================
Hits 13502 13502
Misses 825 825
Partials 571 571 📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
We discussed having a live preview of these changes to proofread them before deploying. I created a repo with the dev build of the docs so that we can see it using github pages. However, it seems that css files from |
However, it seems that css files from _static cannot be queried correctly, I don't know why 🤷
Maybe it's a case of github's jekyll engine interfeering (in jekyll, I
think that files starting with an underscore have a special meaning.
You can add an empty file named ".nojekyll" in the root of the html repo
to try to fix the problem:
https://github.blog/2009-12-29-bypassing-jekyll-on-github-pages/
|
The new theme looks neat! It renders well on my mobile phone using firefox mobile as well. I'm not sure about the content bar (column?) on the right side though. Maybe a better place is to also place it on the left -- although I guess this requires tuning a lot from the theme and not desireable. Also for now the Featured examples links on the main page does not work (all links to the Quick Start page). |
…e/new_doc_theme
Sooo, I still need to make minor changes to the code, but I think we can start reading the documentation! Current version is accessible here: https://nilearn-doc-dev.github.io As discussed during the last nilearn dev meeting, I'll assign some parts of the documentation to read, but feel free to change these assignments! (and to tag people who might be interested) Examples
Other
Our main goals are:
|
Everything in the first- and second-level GLM examples looks good to me. I saw the table of contents issue, but it looks like you're already aware of that. I do feel like the examples sections of the reference pages have a lot of wasted space by having the examples listed vertically. |
Sorry, currently, what issue do you see with the table of content? (it's supposed to be solved haha) Besides, I just found a broken link by chance in https://nilearn-doc-dev.github.io/auto_examples/04_glm_first_level/plot_fixed_effects.html#glm-estimation (link to the nistat's documentation), could you do a quick pass to check there aren't any of these? Thank you! |
This is what I'm seeing: It's still present in https://nilearn-doc-dev.github.io/auto_examples/04_glm_first_level/plot_bids_features.html#sphx-glr-auto-examples-04-glm-first-level-plot-bids-features-py and https://nilearn-doc-dev.github.io/auto_examples/04_glm_first_level/plot_first_level_details.html#sphx-glr-auto-examples-04-glm-first-level-plot-first-level-details-py, at least.
I'm referring to the API documentation. For example, on the page for FirstLevelModel, the examples look like this: In the official documentation, they look like this: It's not a big deal, but there ends up being a lot of empty space with that organization, and some functions or classes that show up in a lot of examples are going to have very long example sections.
I did notice that, but it's also present in the official documentation, so it's a typo in the actual RST page rather than a problem with the new theme. |
Thanks a lot, that's exactly the kind of feedback I need :) |
Soooo, a new version of However, while we're here, we might as well plan some changes for the documentation.
|
…e/new_doc_theme
Thx ! Since your suggestions are big steps, shouldn't we proceed by first merging that one and then reviewing more in detail the documentation ? |
I agree with those points @alexisthual. I am also in favor of merging this first and then working through them. I trust it has been reviewed sufficiently but I can have a look today at the updated docs just to check it overall. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alexisthual, there are a few small things I think we can try to fix before merging.
- The note on this page is in italics and centered. Reformat to look like the others.
- There is an error on this page
- Plots are rendered very small here and also some plots here
- Isn't this module supposed to be called
maskers
instead ofmasking
Also ok if you think some things are better fixed separately in a different PR after merging this. Let me know.
Actually this is reflecting the current documentation and can stay as is. But this related comment should be addressed: #3125 (comment) |
Thanks for your comments @ymzayek, I addressed all of them in my last commit. Point by point:
|
see also #3195 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alexisthual for the quick changes! For me I think it is in a good state to merge.
@alexisthual can you also add an entry to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't identified any big issue in the rendering. Thx a lot !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thx for modernizing the doc !
… into feature/new_doc_theme
When everything passes 🤞 I will have a quick look at the artifacts and then merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexisthual I really think it's almost there! The fix in AUTHORS.rst
should do the trick. See below.
Thanks @alexisthual and all reviewers for your work on this. Merging! |
This PR resolves #3122 and changes the documentation theme to furo for a better looking, more readable and accessible website!
You can preview these changes (without running the examples) by pulling this branch and running:
The main changes are:
./README.md
sphinx-gallery
doc/modules
)doc/modules/references.rst
to new individual pages or just leave this file for now (I added a warning saying this part of the doc is no longer maintainted)examples/00_tutorials
folder for more consistent site structure (this will break some existing urls)requirements-build-docs.txt
(furo
,recommonmark
,sphinx-design
)tune_toc.rst
andbig_toc_css.rst
)Before deploying these changes, we should:
Footnotes
furo
generates two sidebars: the left one shows the website's structure, the right one shows current document's structure. The one to the left is a sphinx table of content generated withtitles_only=True
, which we can't change. This means that the left sidebar represents the exact site structure. If we want examples to be grouped in subsections in the sidebar, we need to create a new nested structure similar to what I did fordoc/modules
. However, examples are generated usingsphinx-gallery
, so we can either wait for asphinx-gallery
issue mentioning nested sections to be solved or generate an individual gallery for each subsection (I'll try the first option quickly and jump to the second if it's not obvious). ↩