-
Notifications
You must be signed in to change notification settings - Fork 352
FIX empty primary sidebar showing up #1682
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
Conversation
@drammock is it possible that you or some other maintainer review this one, if you have time? An empty primary sidebar really affects the look of the website a lot so I'm hoping that this can be included into the next release. Thank you very much 🙏
@gabalafou since you have been doing a lot of work on the TOC navigation and overall accessibility of the theme, could you have a look at this PR (mostly, I want to make sure this does not interfere/introduce potential gotchas against all the work on #1564)
I will give this a review today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are great!
I made one request to fix some comments, but otherwise this PR looks good to me.
Based on the description of PR #1642, I think that this PR will almost completely undo the performance gains of that PR.
It would be good if @drammock could weigh in and explain a line he wrote in his PR description:
The logic is that finding an ancestor and including hidden toctrees together guarantee that a non-empty toc subtree exists for the current page, and we can postpone the toctree.resolve() step until later when it's actually added to the page.
I'm wondering if the logic held for SciPy but just doesn't hold in general, or if there was some misunderstanding about what includehidden
meant.
In any case, we really need to document the sidebar_includehidden
theme configuration variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this test case is at all dependent on the change made in this PR, but it seems like a nice test case to have in the test suite. 😄
@gabalafou Thanks for your review! I've addressed your comment.
Based on the description of PR #1642, I think that this PR will almost completely undo the performance gains of that PR.
agreed
It would be good if @drammock could weigh in and explain a line he wrote in his PR description:
The logic is that finding an ancestor and including hidden toctrees together guarantee that a non-empty toc subtree exists for the current page, and we can postpone the toctree.resolve() step until later when it's actually added to the page.
I'm wondering if the logic held for SciPy but just doesn't hold in general, or if there was some misunderstanding about what
includehidden
meant.
It's entirely possible that I am just wrong about it. My reasoning was:
- either we're on an
:orphan:
page, or we're not - if we're on an
:orphan:
page there will be no ancestor page,ancestorname
will beNone
, and we can suppress the sidebar - if this page is the site root, or the search results page, or some other "special" page, it will also not have an ancestor and we can suppress the sidebar
- if we're not on an orphan or special page, that means this page must show up in a
toctree
directive on some other "higher-level" page. if that is true, then what matters is:- what is
startdepth
, and is current page at least 1 level belowstartdepth
(this is I think handled in theget_ancestor_pagename
function, but I didn't write that code so I'm not 100% sure/confident that it works the way I think it does) - is the global
includehidden
flag True, in which case we can be sure that this page should show up in the sidebar toctree (regardless of whether it's added in atoctree
that does or does not have the:hidden:
flag).
- what is
One of those last two sub-bullets is probably wrong. Maybe the theme-level includehidden
setting is not getting passed through correctly? Sadly I'm about to be AFK for ~1.5 weeks so I can't dig any deeper at the moment.
My earlier look at @djhoese's site showed an empty toc-item
DIV getting added; if someone were to dig deeper while I'm away, that's where I'd start I guess.
Fixes #1662. For more information see the referenced issue.
I also added a minimal reproducible site for testing, not sure if it is proper.
A side effect is that we may lose some speed gain in #1642, not sure how much.