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

Update docs #2373

Merged
merged 38 commits into from Jul 20, 2023
Merged

Update docs #2373

merged 38 commits into from Jul 20, 2023

Conversation

aknierim
Copy link
Contributor

@aknierim aknierim commented Jul 4, 2023

Docs are now built with Sphinx 6.2.1 and the PyData theme

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@aknierim aknierim marked this pull request as draft July 4, 2023 13:43
* Added PyData theme
* Modified front page
* Fixed warnings during build of the docs with the Sphinx 6.2.1
* Added version switcher
* Fixed warnings by changing ``automodapi`` to ``automodule`` at certain
  locations
* Added change log rst file
* Removed unused import of ``pydata_sphinx_theme``
* Updated environment.yml
* Added ``:noindex:`` to get rid of warnings
* Works at least locally
docs/conf.py Outdated Show resolved Hide resolved
* use autosummary and some manually added titles instead of automodapi
  in ``ctapipe_api/instrument/camera.rst`` and ``ctapipe_api/calib/index.rst``
* allows to circumvent duplicate reference warnings
* removed ``nipick_ignore`` entries added in previous commits of this
  ``update-docs`` branch
Not adding to ``__all__`` in ``ctapipe/calib/index_camera/__init__.py``
and ``ctapipe/instrument/camera/__init__.py`` fixes nitpick warnings
with automodapi. Note that importing the modules is still necessary.
* Change back to ``automodapi`` in ``ctapipe_api/calib/index.rst`` and
  ``ctapipe_api/instrument/camera.rst``
* Previous workaround no longer necessary
* Fix mobile layout of front page (grid cards now display correctly)
* Cleaner docs front page
Ignore unused import warnings in ``ctapipe/calib/camera/__init__.py``
``ctapipe/instrument/camera/__init__.py`` using ``noqa: F401``.
This will allow the linting CI to run successfully, while still allowing
the docs to be built with ``automodapi``.
@aknierim aknierim marked this pull request as ready for review July 10, 2023 10:17
@aknierim aknierim requested a review from watsonjj as a code owner July 10, 2023 10:17
@kosack
Copy link
Contributor

kosack commented Jul 10, 2023

For review: here is the RTD link for this PR

environment.yml Outdated
@@ -47,5 +48,5 @@ dependencies:
- zlib
- zstandard
- eventio>=1.9.1
- jinja2=3.0 # for sphinx 3.5, remove when updating to 4.x
- jinja2
Copy link
Member

Choose a reason for hiding this comment

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

can be completely removed

kosack
kosack previously approved these changes Jul 17, 2023
* Remove version switcher since it needs to be updated by hand every time
a new version is released

* Remove now-obsolete ``docs/_static/switcher.json``

* Remove ``jinja2`` dependency from ``environment.yml``
@aknierim
Copy link
Contributor Author

Removed the version switcher from the navbar per request by @maxnoe as it has to be updated by hand every time a new version is released.

@aknierim aknierim requested a review from maxnoe July 17, 2023 14:16
docs/conf.py Outdated
],
}

html_sidebars = {"**": ["sidebar-nav-bs.html", "sidebar-ethical-ads.html"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a waste of space: there is no content in the sidebar for most of the pages

Copy link
Member

Choose a reason for hiding this comment

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

The version switcher is there. We had an alternative version, but that would require manually curating a json file of versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right, but this is currently the only way to get the version switcher from RTD. The way I see it, we currently have three options:

  1. We go back and reimplement the dropdown menu I previously added. This however would mean that we have to add any new version to a JSON file by hand.

  2. We use the flyout menu from RTD, thus making it necessary to have the sidebar on all pages.

  3. We use the flyout menu only on specific pages like the front page and for example the API docs.

I'm not sure which option is the best as I guess all of them have their drawbacks...

Copy link
Contributor

@kosack kosack Jul 18, 2023

Choose a reason for hiding this comment

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

Hmm, this works fine in the vodf pages I created.. Not sure how I configured it, but there the switcher is floating on the lower-right corner: https://vodf.readthedocs.io/en/latest/ (and also in the top-bar)
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course the ethical ads don't appear though

Copy link
Contributor Author

@aknierim aknierim Jul 18, 2023

Choose a reason for hiding this comment

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

I tried to figure out why the behaviour is so different from your vodf pages today but couldn't find anything. My only guess would be different behaviour when using Sphinx 4.5 and PyData 0.10 for the vodf pages versus the ctapipe docs with Sphinx 6.2.1 and PyData 0.13.3...

Copy link
Contributor Author

@aknierim aknierim Jul 18, 2023

Choose a reason for hiding this comment

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

The version switcher in the navbar on the vodf pages is of course the same one that uses the manually curated json file. But the version switcher from RTD is what boggles my mind.

At least from what I gathered from the PyData theme dev docs and an open issue, they are currently working on a way to make the RTD version switcher more customizable, or at least movable from the sidebar. But for the time being it should actually only appear in the primary sidebar (if not hidden) on the left.

Copy link
Contributor

Choose a reason for hiding this comment

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

Who knows? I can try updating the VODF docs and see if it disappears... You're right that it's not quite the same version.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we can deal with that in a later PR, for now it's fine.

* Move content of caution admonition block to announcement banner, as
  this results in a cleaner main page

* Remove ``html_sidebars`` for testing purposes
At least we have a version switcher for now
@maxnoe
Copy link
Member

maxnoe commented Jul 19, 2023

Okay, maybe we just bite the bullet and have the manual switcher. Sorry for going back and forth.

It looks much better, doesn't have the sidebar space issue and we already have manual steps in the release process like rendering the changelog.

So let's maybe go for that (and add a section on "how to make a release" to the developer documentation).

I can do that as part of #2383 , after we merge here.

docs/_static/switcher.json Outdated Show resolved Hide resolved
* Stable is now called stable (without version tag)
* Add colors to stable and dev versions in switcher
@kosack
Copy link
Contributor

kosack commented Jul 20, 2023

The only thing I would suggest is that maybe the top-level admonition about the instability is a bit much: it takes up a lot of page space. Just having it as a disclaimer on the main page is probably fine. Most people understand that a 0.X release is unstable

@maxnoe
Copy link
Member

maxnoe commented Jul 20, 2023

The only thing I would suggest is that maybe the top-level admonition about the instability is a bit much: it takes up a lot of page space.

I said the same, but we discussed that most people probably don't enter through the main page. I don't have a strong opinion on this.

Let's merge here so we have an up-to-date sphinx and a better design and do more content changes in other PRs.

@maxnoe maxnoe merged commit c7f5ee2 into cta-observatory:main Jul 20, 2023
12 of 13 checks passed
@aknierim aknierim deleted the update-docs branch July 20, 2023 12:08
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.

None yet

3 participants