-
Notifications
You must be signed in to change notification settings - Fork 317
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
ENH: Add parameters navigation_startdepth
and toc_caption_maxdepth
#1241
base: main
Are you sure you want to change the base?
Conversation
Allows users to start the sidebar Toc at level 0 Addresses issue pydata#1181
Copy function from toctree.py that needs to be modified (as-is for prettier diffs in future commits)
Rewrite toctree.resolve to an internal function that accepts `toc_caption_maxdepth` as a keyword argument
Use the new local _resolve function and pass it a new keyword argument: `toc_caption_maxdepth`.
Edit example pages and show commented out theme-parameters in conf.py
Change heading of sidebar-nav depending on `theme_navigation_startdepth`. Shows "Site Navigation" when startdepth=0 or "Section Navigation" otherwise
…nd `toc_caption_maxdepth`. Add gif to show intended behavior of collapsible toc captions.
I have now added documentation for the new options. For now I've added notes about this in the documentation rather than fixing the issues with |
@SimenZhor I just want to say thanks for working on this and sorry I haven't had a chance to review it yet, will try to get to it early next week. |
Use `_traverse_or_findall` function instead of assuming `findall` is present.
Remove test for references to non-included documents. This feature was introduced sometime between Sphinx 5 and 6 apparently.
When `TocTree._toctree_prune` does not exist, use approach from Sphinx 6.1.3 for pruning the toctree.
@drammock Awesome! And no problem :) I think I have fixed most of the test-failures now by the way. So it would be nice if you could re-trigger the CI pipeline to see if I've missed something. Locally I'm able to run It was a bit confusing to debug those test failures by the way. |
looks like all the CIs passed except lint (expected, since you disabled pre-commit on this PR). As a first pass, here are some questions/comments:
|
Thanks for the good feedback, @drammock 1.
This is my first PR to this repo, so my main motivation behind this choice was to avoid breaking changes. I do agree with your observation that it does seem like a bugfix, so I wouldn't vote against removing the However, as noted earlier, my current implementation will introduce a quite nasty bug for That being said, I do have a working proof-of-concept on this branch, but a side effect is that it introduces a new toctree-level for the captions themselves, which means that the behavior of 2.I'm not addressing the second part of #944 (header with only custom links). Do you know if that has been addressed elsewhere already? I think you are right that it closes #221, but that one is discussing many other, somewhat unrelated, design choices as well. I think this PR will address the original issue as it was described though, so maybe one of the stakeholders from the thread could take a closer look at the discussion - and open separate issues for anything that remains open? I'll add 3.Good observations. I'll fix those. |
dang. I didn't catch that before.
my instinct is that we shouldn't be introducing new TOC levels for captions. Maybe if we can do it in a way that is fully invisible to the end user... but it seems likely to be complicated to implement and thus prone to nasty bugs. I'll try to find some time to play around with this, and see if I can help figure it out. |
I totally agree. I've been trying to get rid of this side effect without luck so far. I don't remember all the details off the top of my head, but I believe the reason it happens is because I'm packing the list items ( The system with I'm not very proficient with HTML, but it feels like I'm quite close and that there is an obvious solution I'm not seeing (like swapping out
I would truly appreciate some help, or pointers on what to try :) I won't be able to look more at this until next week at the earliest. |
Gah! This critical improvement never happened, merge conflicts invariably cropped up, and now everybody is tired. This saddens me. At least we always have |
@leycec thank you for you for this very constructive and somewhat disrespectful comment. Note that if this functionality is critical to you, we will appreciate any contributions. @drammock, @SimenZhor thanks for all the work you did so far and I hope we will eventully merge it. Don't loose hope our record is currently this one: #399 that we eventually merged in smaller pieces. |
Hi,
As discussed in Issue #1181 it is quite straight forward to implement a new parameter that allows users to decide which level the sidebar TOC should start at. By introducing a new global theme-parameter,
navigation_startdepth
, that is passed to thegenerate_toctree_html
function. I have made a separate branch here that showcases only this functionality.Here is a screenshot showing all top-level TOCs collapsed (
navigation_startdepth=0
):As I developed this on my own project, everything worked as expected. But when I tested it on the pydata-sphinx-theme docs I noticed that setting the startdepth to 0 would break the nice looking subcategories generated by the
toctree
:captions:
, as these were only rendered for the first layer of toctrees (discussed in: #192 and #135)Screenshots:
So, to avoid breaking this feature for
navigation_startdepth=0
I've also introduced a parameter for specifying how deep the toctree captions should be rendered:toc_caption_maxdepth
.Here is a screenshot of how it looks when
toc_caption_maxdepth
is set to a high number:I had to copy and edit yet another giant function from the Sphinx library into the monolithic
__init__.py
, so I've not yet written any documentation for these new parameters as I wanted a "go" from one of the maintainers before putting more time into this.NOTE: In an effort to keep my changes to this Sphinx-function readable, I had to disable
pre-commit
checks, as it insisted on reformatting the entire__init__.py
.Closes #1181
Other relevant issues: #944 #221