Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
Charlie-XIAO wants to merge 4 commits into pydata:main
base: main
Choose a base branch
Loading
from Charlie-XIAO:sidebar-appear-nothing

Conversation

Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Jan 28, 2024

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.

Copy link
Contributor Author

@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 🙏

Copy link
Collaborator

@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)

Charlie-XIAO reacted with thumbs up emoji

@trallard trallard added the tag: component Issues or improvements associated with a given component in the theme label Feb 12, 2024
Copy link
Collaborator

I will give this a review today

Charlie-XIAO reacted with heart emoji

Copy link
Collaborator

@gabalafou gabalafou left a 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.

Charlie-XIAO reacted with eyes emoji
Some text.

.. toctree::
:hidden:
Copy link
Collaborator

@gabalafou gabalafou Feb 13, 2024

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. 😄

Charlie-XIAO reacted with laugh emoji
Copy link
Contributor Author

Charlie-XIAO commented Feb 13, 2024
edited
Loading

@gabalafou Thanks for your review! I've addressed your comment.

Copy link
Member

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 be None, 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 below startdepth (this is I think handled in the get_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 a toctree that does or does not have the :hidden: flag).

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabalafou gabalafou gabalafou requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

tag: component Issues or improvements associated with a given component in the theme

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Section navigation shows up even if it contains nothing

AltStyle によって変換されたページ (->オリジナル) /