Skip to content

Use pydata-sphinx-theme#14867

Merged
pllim merged 8 commits into
astropy:mainfrom
pllim:yessss-dark-mode-aka-pr14477-try2
Aug 1, 2023
Merged

Use pydata-sphinx-theme#14867
pllim merged 8 commits into
astropy:mainfrom
pllim:yessss-dark-mode-aka-pr14477-try2

Conversation

@pllim

@pllim pllim commented May 24, 2023

Copy link
Copy Markdown
Member

Description

This pull request is to supersede #14477 and fix #11520 . I moved some common stuff upstream to sphinx-astropy and managed to get this working without moving all the docs around.

Rendered doc: https://astropy--14867.org.readthedocs.build/en/14867/

Pros:

  • Dark mode!
  • Use a theme that is also used by other Python libraries (e.g., numpy, matplotlib). It has mobile support, is quite responsive, and did I say dark mode?

Cons (I don't think it is a deal breaker):

Requires:

TODO:

  • Wait for sphinx-astropy release with required feature.
  • Remove hack in setup.cfg.

Follow-up action items after merge:

  • Send out communication about theme change and map the old user guide access to new (maybe screenshots of both and some annotation between them?).

@pllim pllim added this to the v6.0 milestone May 24, 2023
@github-actions

github-actions Bot commented May 24, 2023

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@pllim

pllim commented May 25, 2023

Copy link
Copy Markdown
Member Author

pre-commit.ci autofix

@pllim

This comment was marked as resolved.

@pllim

pllim commented May 25, 2023

Copy link
Copy Markdown
Member Author

Thanks for all your help, @drammock ! If you want, I can add you as co-author on the second commit. Just have to give me the info I can link to your GitHub account (Name <email>).

@drammock

Copy link
Copy Markdown
Contributor

Thanks for all your help, @drammock ! If you want, I can add you as co-author on the second commit. Just have to give me the info I can link to your GitHub account (Name <email>).

Co-authored-by: Daniel McCloy <dan@mccloy.info>

Thanks! Probably the only time I'll know enough to contribute to astropy 😅

@saimn

saimn commented May 30, 2023

Copy link
Copy Markdown
Contributor

Thanks for working on that @pllim :)
There is a thing that is missing compared to the old PR (maybe due to the toctree tweaks): the theme can show the toctree of the subpackage when browsing a subpage, in the left bar, e.g.
image
This is very useful to discover other subsections, but it's not showing up in the new version.
old: https://astropy--14477.org.readthedocs.build/en/14477/user/units/quantity.html
new: https://astropy--14867.org.readthedocs.build/en/14867/units/quantity.html

@saimn

saimn commented May 30, 2023

Copy link
Copy Markdown
Contributor

That must be because of the new levels of toctrees. I think the navigation between subsections/subpackages with the sidebar was clearer in the old version.
Also,

  • not sure if we need the sidebar in the index page, it duplicates the content of the page
  • a lot of the space in the index page is taken by less important things (e.g. the Development section) whereas the most important (the list of subpackages) is hidden.

@pllim

pllim commented May 30, 2023

Copy link
Copy Markdown
Member Author

I think the navigation between subsections/subpackages with the sidebar was clearer in the old version

I struggle with this one because it is tough to retain the old behavior without moving the files around (as you did, but the it got conflicts very quickly). If we were to do it your way, this PR will have to get merged really fast after I make move the files. What do you think? Is there some other sidebar CSS trick or something that I need to know here?

a lot of the space in the index page is taken by less important things (e.g. the Development section) whereas the most important (the list of subpackages) is hidden

Same problem as above. A side effect of my trying not to move files around. So I guess I'll either have to move files like you did in #14477 or figure out a clever way to lay out the TOC.

not sure if we need the sidebar in the index page

The original theme doesn't include it. I actually included this one because on my small laptop screen, I had to scroll through a lot to get to the bottom of the index page and found discovering via sidebar easier. But now that I am back on normal PC monitor, I see your point. I can remove it.

@pllim

pllim commented May 30, 2023

Copy link
Copy Markdown
Member Author

Probably the only time I'll know enough to contribute to astropy

Don't be silly, @drammock ! 😸

@taldcroft

Copy link
Copy Markdown
Member

My initial review of the output is that this looks fantastic. I strongly endorse moving forward with this effort!

@saimn

saimn commented Jun 2, 2023

Copy link
Copy Markdown
Contributor

I struggle with this one because it is tough to retain the old behavior without moving the files around (as you did, but the it got conflicts very quickly). If we were to do it your way, this PR will have to get merged really fast after I make move the files. What do you think?

I think the most important is that the docs are well organized and easy to browse, for users. So if we need to move files for that and we don't have a good alternative, then we just need to do it. If there is an agreement then we can do that quickly enough to avoid conflicts I guess.

@pllim pllim force-pushed the yessss-dark-mode-aka-pr14477-try2 branch 2 times, most recently from 3afdd9b to d41da58 Compare June 2, 2023 22:22
@pllim

pllim commented Jun 2, 2023

Copy link
Copy Markdown
Member Author

@saimn , I think I figured it out. It was a matter of where to put the headings and how many levels of sub-headings to expose in TOC. A little tricky but I think the layout now closely matches what you came up with in #14477 without moving files around, at least when I built it locally. Let's see if RTD agrees.

@mwcraig

mwcraig commented Jul 8, 2023

Copy link
Copy Markdown
Member

Looking forward to this quite a bit -- thanks to everyone who made it happen!

@eteq

eteq commented Aug 1, 2023

Copy link
Copy Markdown
Member

In general I'm liking this now, but one question: is the intent to keep all the old docs as they were, and switch to this on dev (and then presumably it's "offical" in the next release)? Or to try to push the theme back to the older versions as well?

I'm in favor of it "from now on" rather than trying to change any older versions, which I think is the intent, but just confirming.

@pllim

pllim commented Aug 1, 2023

Copy link
Copy Markdown
Member Author

Or to try to push the theme back to the older versions as well?

I have zero intention nor time to try to backport this. So, this new theme won't be "released" until v6 is out by the end of 2023.

@eteq

eteq commented Aug 1, 2023

Copy link
Copy Markdown
Member

Ok awesome, then I like where it is now! I still have some color quibbles (I we should use "Astropy orange" in some places) but I don't think that needs to block this PR, it can be a follow-up PR once this is in.

pllim and others added 8 commits August 1, 2023 13:41
Co-authored-by: Simon Conseil <contact@saimon.org>
DOC: Fix main title

Install optional deps from sphinx-astropy now

Co-authored-by: Daniel McCloy <dan@mccloy.info>
because it is repetitive

Fix TOC layout

Fix reference API TOC

Fix pre-commit complains.

Use newly released sphinx-astropy.
Fix comma

Use cards on index page [ci skip]
that is copied over from astropy-sphinx-theme

Fix link of project details card.

pre-commit needs to cool it down
@pllim pllim force-pushed the yessss-dark-mode-aka-pr14477-try2 branch from cbdc40e to 5fda110 Compare August 1, 2023 17:46

@nstarman nstarman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree. Great to get this out and iterate from there. Thanks @pllim!

@pllim pllim enabled auto-merge August 1, 2023 18:23
@pllim

pllim commented Aug 1, 2023

Copy link
Copy Markdown
Member Author

Thanks! For the record: 3/5 CoCo members approved (I have to abstain and Derek is on vacation).

@pllim

pllim commented Aug 1, 2023

Copy link
Copy Markdown
Member Author

It failed with a warning that is unrelated (numpy/numpy.org#664) but it does block RTD from building. 😭

WARNING: The following HTTPError has occurred fetching https://numpy.org/doc/stable//: 404 (Not Found)

@pllim pllim disabled auto-merge August 1, 2023 20:42
@pllim

pllim commented Aug 1, 2023

Copy link
Copy Markdown
Member Author

RTD passed but didn't report back. I am just gonna merge manually. Thanks!

@pllim pllim merged commit 16d91dc into astropy:main Aug 1, 2023
@pllim pllim deleted the yessss-dark-mode-aka-pr14477-try2 branch August 1, 2023 20:43
@dhomeier

dhomeier commented Aug 1, 2023

Copy link
Copy Markdown
Contributor

Thanks for getting this done! I do agree to some extent with #14867 (comment), although having both Notes and lengthy Warnings is a particularly disadvantageous example, and I also find the box rendering in the new docs much more pleasant on the eyes.
Section headings font sizes indeed seem quite excessive, so there probably is some fine-tuning to be done in the follow-up.

@pllim

pllim commented Aug 1, 2023

Copy link
Copy Markdown
Member Author

Most section heading levels are inherited from the old theme, so I think modifying the CSS rather than changing the RST would probably be less disruptive. I will open issue (#15112).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DOC: pydata sphinx theme