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

📚 New theme & rearrangement of the docs #4835

Merged
merged 67 commits into from
Mar 15, 2021

Conversation

andersy005
Copy link
Member

@andersy005 andersy005 commented Jan 21, 2021

  • Closes #xxxx
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@andersy005
Copy link
Member Author

Cc @jhamman

@andersy005 andersy005 marked this pull request as ready for review January 21, 2021 05:54
@andersy005
Copy link
Member Author

@pydata/xarray, does anyone know what's going on with readthedocs: https://readthedocs.org/projects/xray/builds/12928622/? I can't seem to figure out the error causing the build failures.

@keewis
Copy link
Collaborator

keewis commented Feb 3, 2021

that's because of the headings in whats-new.rst. I think you have to be consistent with the style you use (=, +, ~, -).

@DocOtak
Copy link
Contributor

DocOtak commented Feb 3, 2021

Also, the build step also has a -W flag which turns warnings into Errors and causes a non 0 exit status. This is probably because the read the docs config file for xarray has a fail_on_warning set to true

@andersy005
Copy link
Member Author

andersy005 commented Feb 3, 2021

that's because of the headings in whats-new.rst. I think you have to be consistent with the style you use (=, +, ~, -).

Ah okay... Is this issue connected to a specific sphinx version or readthedocs? I'm asking because I don't run into this issue locally

Edit: I was too quick to reply. 👍🏽 for @DocOtak's explanation (#4835 (comment))

@andersy005
Copy link
Member Author

Also, the build step also has a -W flag which turns warnings into Errors and causes a non 0 exit status. This is probably because the read the docs config file for xarray has a fail_on_warning set to true

Thank you for the clarification....

@andersy005
Copy link
Member Author

@dcherian,

Can we add another level of organization to API reference in the sidebar: "Top-Level Functions", "Dataset" etc.?

It'd be nice to get the left sidebar to "pin" like the right one. Right now if you scroll enough ,it disappears

I have yet to figure this out. If someone knows how to do this (whether with or without custom HTML template), please let me know :)

Another minor comment: How about renaming "external resources" to "Tutorials & Videos"? Similarly "Related projects" to "Ecosystem"?

Done!

@keewis,

The only issue I have with the new theme is that due to the maximum line length we use some examples have horizontal scroll bars (unless the left side bar is hidden).

I will see if there's a remedy for this.

doc/conf.py Outdated Show resolved Hide resolved
@keewis
Copy link
Collaborator

keewis commented Mar 7, 2021

I will see if there's a remedy for this.

You could try to reduce the (quite big) margin on the left / right

@andersy005
Copy link
Member Author

It'd be nice to get the left sidebar to "pin" like the right one. Right now if you scroll enough ,it disappears

@dcherian, this is fixed. When you get a moment, take another look and let me know if I missed anything.

@andersy005
Copy link
Member Author

The only issue I have with the new theme is that due to the maximum line length we use some examples have horizontal scroll bars (unless the left side bar is hidden).

@keewis, I fixed this issue.

@pydata/xarray, this PR is ready. Unless there's any additional issue that needs to be addressed before merging, feel free to merge this.

Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

@keewis, I fixed this issue.

thanks, I can confirm this is fixed.

Looking at pydata-sphinx-theme and sphinx-book-theme, both seem to have a doc_path or path_to_docs context variable which allows specifying a custom path (I think we can merge this PR in its current state and fix this and any other issues in a new PR, though). Even if we get the edit-on-github feature to work I would keep the source link: it is quite useful for development (especially for reviewing and answering issues & discussions).

The reasoning for the current behavior of the edit-on-github feature was that people were opening PRs to be merged into stable, which would then be dropped on a release. We could avoid that by either always linking to master (but then we can't determine the line numbers; this is what we are doing with scanpydoc), or by always opening a PR which also adds the commit to master (using a bot?). Another option would be to restrict PRs into stable to maintainers, but I'm not sure if github supports that.

Comment on lines +209 to +228
html_theme = "sphinx_book_theme"
html_title = ""

html_context = {
"github_user": "pydata",
"github_repo": "xarray",
"github_version": "master",
"doc_path": "doc",
}

# Theme options are theme-specific and customize the look and feel of a theme
# further. For a list of options available for each theme, see the
# documentation.
html_theme_options = {"logo_only": True}

# Add any paths that contain custom themes here, relative to this directory.
# html_theme_path = []

# The name for this set of Sphinx documents. If None, it defaults to
# "<project> v<release> documentation".
# html_title = None
html_theme_options = dict(
# analytics_id='' this is configured in rtfd.io
# canonical_url="",
repository_url="https://github.com/pydata/xarray",
repository_branch="master",
path_to_docs="doc",
use_edit_page_button=True,
Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at pydata-sphinx-theme and sphinx-book-theme, both seem to have a doc_path or path_to_docs context variable which allows specifying a custom path (I think we can merge this PR in its current state and fix this and any other issues in a new PR, though).
The reasoning for the current behavior of the edit-on-github feature was that people were opening PRs to be merged into stable, which would then be dropped on a release. We could avoid that by either always linking to master (but then we can't determine the line numbers; this is what we are doing with scanpydoc), or by always opening a PR which also adds the commit to master (using a bot?)

@keewis, unless I am misunderstanding how the existing edit on Github button works, I think these settings allow us to replicate the existing behavior i.e. For (1) regular pages, the button links to the document/file in the master branch. For (2) API pages, the button is disabled and the [source] link links to the lines of code in the master branch. Could you elaborate on when/how with the new changes, the edit on GitHub button will result in pointing people to the wrong branch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the edit on github link will currently point to the same branch / tag / commit the docs were built for (regardless of the page).

The existing fix (always point to master) makes it impossible to accurately determine the line numbers for API pages. Since we might even have removed that function I'm starting to think that it might be easier to not redirect at all but rather make the fact that the PR does not point to master more visible.

Not sure what the best way to do so is, though. Maybe have a workflow post a comment if the branch is not the dev branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the clarification!

Not sure what the best way to do so is, though. Maybe have a workflow post a comment if the branch is not the dev branch?

I presume it'd be okay to merge this PR and then address this in a separate PR in the near future?

Copy link
Collaborator

Choose a reason for hiding this comment

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

for sure!

@andersy005
Copy link
Member Author

I plan to merge this later today (unless there's any objection and/or necessary changes)

@shoyer
Copy link
Member

shoyer commented Mar 15, 2021 via email

@andersy005
Copy link
Member Author

Thank you @keewis, @dcherian, @shoyer!

@andersy005 andersy005 changed the title New theme & rearrangement of the docs 📚 New theme & rearrangement of the docs Mar 15, 2021
@andersy005 andersy005 merged commit 76d6d75 into pydata:master Mar 15, 2021
@andersy005 andersy005 deleted the docs/new-theme branch March 15, 2021 23:20
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 18, 2021
…indow

* upstream/master:
  Fix regression in decoding large standard calendar times (pydata#5050)
  Fix sticky sidebar responsiveness on small screens (pydata#5039)
  Flexible indexes refactoring notes (pydata#4979)
  add a install xarray step to the upstream-dev CI (pydata#5044)
  Adds Dataset.query() method, analogous to pandas DataFrame.query() (pydata#4984)
  run tests on python 3.9 (pydata#5040)
  Add date attribute to datetime accessor (pydata#4994)
  📚 New theme & rearrangement of the docs (pydata#4835)
  upgrade ci-trigger to the most recent version (pydata#5037)
  GH5005 fix documentation on open_rasterio (pydata#5021)
  GHA for automatically canceling previous CI runs (pydata#5025)
  Implement GroupBy.__getitem__ (pydata#3691)
  conventions: decode unsigned integers to signed if _Unsigned=false (pydata#4966)
  Added support for numpy.bool_ (pydata#4986)
  Add additional str accessor methods for DataArray (pydata#4622)
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 23, 2021
…-tasks

* upstream/master:
  Fix regression in decoding large standard calendar times (pydata#5050)
  Fix sticky sidebar responsiveness on small screens (pydata#5039)
  Flexible indexes refactoring notes (pydata#4979)
  add a install xarray step to the upstream-dev CI (pydata#5044)
  Adds Dataset.query() method, analogous to pandas DataFrame.query() (pydata#4984)
  run tests on python 3.9 (pydata#5040)
  Add date attribute to datetime accessor (pydata#4994)
  📚 New theme & rearrangement of the docs (pydata#4835)
  upgrade ci-trigger to the most recent version (pydata#5037)
  GH5005 fix documentation on open_rasterio (pydata#5021)
  GHA for automatically canceling previous CI runs (pydata#5025)
  Implement GroupBy.__getitem__ (pydata#3691)
  conventions: decode unsigned integers to signed if _Unsigned=false (pydata#4966)
  Added support for numpy.bool_ (pydata#4986)
  Add additional str accessor methods for DataArray (pydata#4622)
  add polyval to polyfit see also (pydata#5020)
  mention map_blocks in the docstring of apply_ufunc (pydata#5011)
  Switch backend API to v2 (pydata#4989)
  WIP: add new backend api documentation (pydata#4810)
  pin netCDF4=1.5.3 in min-all-deps (pydata#4982)
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.

5 participants