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.

Sign up for GitHub

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

Jump to bottom

maybe fix missing sidebar? #1632

Merged
merged 5 commits into from
Jan 9, 2024
Merged

maybe fix missing sidebar? #1632

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d359930
maybe fix missing sidebar?
drammock Jan 5, 2024
bd8f789
simplify
drammock Jan 8, 2024
57728c9
purge internal defaults & use theme config setting
drammock Jan 8, 2024
376bfba
formatting
drammock Jan 8, 2024
867dac1
refactor and simplify toctree code
drammock Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions src/pydata_sphinx_theme/toctree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,14 @@
def get_unrendered_local_toctree(
app: Sphinx, pagename: str, startdepth: int, collapse: bool = True, **kwargs
):
"""."""
if "includehidden" not in kwargs:
kwargs["includehidden"] = False
"""Get the "local" (starting at `startdepth`) TOC tree containing the current page.

This is similar to `context["toctree"](**kwargs)` in sphinx templating,
but using the startdepth-local instead of global TOC tree.
"""
kwargs.setdefault("includehidden", True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the key change. it was copy/pasted from its old place within index_toctree in #1609 and I don't really know why it was previously setting False as the default. I'm not even sure that we should be forcing it here; it might be better to leave it unset (or better yet: to use the theme setting sidebar_includehidden)

Copy link
Collaborator

Choose a reason for hiding this comment

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

My feeling is that includehidden is sensible to default to True, so I'm +1 on this. A common use-case is to use hidden to not show the TOCtree on the page but people do want it in the sidebar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but see #1551

Copy link
Collaborator

Choose a reason for hiding this comment

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

That just means we need it to be possible to not include them, but doesn't change your opinion that the default should be to show them, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed the default behviour is to include them in all the other theme I know so Users (and us) would be surprised not to see the complete toctree in the sidebar (again by default)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That just means we need it to be possible to not include them, but doesn't change your opinion that the default should be to show them, right?

Yes. But that means we need to use theme_sidebar_includehidden somewhere where is currently not being used, so that it ends up in these kwargs. I think it will work to do that in layout.html as in the linked PR but I haven't had time to do thorough testing/debugging yet.

But it's helpful to know that this fixes things for @lwasser --- it means we're on the right track

if kwargs.get("maxdepth") == "":
kwargs.pop("maxdepth")

Check warning on line 45 in src/pydata_sphinx_theme/toctree.py

View check run for this annotation

Codecov / codecov/patch

src/pydata_sphinx_theme/toctree.py#L45 

Added line #L45 was not covered by tests

toctree = TocTree(app.env)
if sphinx.version_info[:2] >= (7, 2):
Expand All @@ -47,7 +50,7 @@

ancestors = [*_get_toctree_ancestors(app.env.toctree_includes, pagename)]
else:
ancestors = toctree.get_toctree_ancestors(pagename)

Check warning on line 53 in src/pydata_sphinx_theme/toctree.py

View check run for this annotation

Codecov / codecov/patch

src/pydata_sphinx_theme/toctree.py#L53 

Added line #L53 was not covered by tests
try:
indexname = ancestors[-startdepth]
except IndexError:
Expand Down Expand Up @@ -457,12 +460,10 @@
doctree = self.env.tocs[indexname].deepcopy()

toctrees = []
if "includehidden" not in kwargs:
kwargs["includehidden"] = True
kwargs.setdefault("includehidden", True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unrelated simplification that I noticed in passing

if "maxdepth" not in kwargs or not kwargs["maxdepth"]:
kwargs["maxdepth"] = 0
else:
kwargs["maxdepth"] = int(kwargs["maxdepth"])
kwargs["maxdepth"] = int(kwargs["maxdepth"])
drammock marked this conversation as resolved.
Show resolved Hide resolved
kwargs["collapse"] = collapse

# TODO: use `doctree.findall(addnodes.toctree)` once docutils min version >=0.18.1
Expand Down
Loading