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

Empty TOC in right sidebar #854

Closed
tupui opened this issue Aug 2, 2022 · 16 comments · Fixed by #1115
Closed

Empty TOC in right sidebar #854

tupui opened this issue Aug 2, 2022 · 16 comments · Fixed by #1115
Assignees
Milestone

Comments

@tupui
Copy link
Contributor

tupui commented Aug 2, 2022

When the TOC is empty, the right sidebar is still present through bd-sidebar-secondary bd-toc

181444254-c3d2ca92-5238-46cc-9a78-068a798b0317

This is observed in SciPy. To build SciPy and the doc (build should take around 3 mins with 6 cores, then 5 mins for the doc itself):

# once you cloned my branch: https://github.com/scipy/scipy/pull/16660
# create an conda env to install everything
# don't try on windows as it will mostly fail.
mamba create env -f environment.yml
python dev.py doc -j 6  # number of cores
@drammock drammock added the impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship label Aug 2, 2022
@jarrodmillman jarrodmillman added this to the 0.10 milestone Aug 4, 2022
@choldgraf
Copy link
Collaborator

Can somebody please provide a minimal reproducible example or link? I worry that this won't get fixed if debugging requires building the entire scipy docs...

@drammock
Copy link
Collaborator

I've put in a PR to MNE-Python that should generate a working CircleCI site that demos this error. Will update here once the build is done.

@drammock
Copy link
Collaborator

Here is the rendered MNE-Python doc with the empty right column:

@choldgraf
Copy link
Collaborator

choldgraf commented Aug 13, 2022

Diagnosis of what's going wrong

OK looking at the docs that @drammock shared, I think that this is the problem:

  • We have two extra toc items enabled by default: edit-this-page, and sourcelink (ref). These are all added to theme_page_sidebar_items.
  • In addition, we provide configuration flags to trigger this behavior on and off (ref).
  • If people trigger edit-this-page or sourcelink to be False, then they won't show up, but their **template will still be in theme_page_sidebar_items. As a result, two empty <div>s will be created corresponding to the empty template of each.

What we can do to improve this

Short-term fix

First off, I think that @drammock and @tupui can solve their problem by defining:

html_theme_options = {
  ...
  "secondary_sidebar_items": ["page-toc"],
  ...
}

This should remove those empty <div> elements entirely.

Long-term fix

Ultimately, I think the behavior that we would want is:

  • Setting any of the config for these features to False should also remove it from the secondary_sidebar_items list. Similar to what we do with the primary sidebar if there are no navigation entries to display.
  • If there are no entries in the secondary-sidebar, then the whole sidebar should not show up at all, and allow extra space for the content.
  • Either way, we need to document all of these more clearly!

If somebody wants to give a shot at the long-term fix, I'm happy to review!

@tupui
Copy link
Contributor Author

tupui commented Aug 15, 2022

I will try the work around tomorrow thanks!

@tupui
Copy link
Contributor Author

tupui commented Aug 17, 2022

I can confirm that it does fix the issue (and does not seem to have unwanted side effect for me), thanks! 😃

@drammock
Copy link
Collaborator

html_theme_options = {
  ...
  "page_sidebar_items": ["page-toc"],
  ...
}

agreed that this does fix things for MNE-Python docs too. I'll see if I can figure out the long-term fix.

@drammock drammock self-assigned this Aug 17, 2022
@drammock
Copy link
Collaborator

Also since there's a very easy workaround (just one line change to conf.py) I'm comfortable removing the block-release tag on this one.

@drammock drammock removed the impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship label Aug 17, 2022
@jarrodmillman jarrodmillman modified the milestones: 0.10, 0.11 Aug 29, 2022
@jarrodmillman jarrodmillman modified the milestones: 0.11, 0.12 Oct 4, 2022
@12rambau 12rambau modified the milestones: 0.12, 0.13 Nov 17, 2022
@choldgraf
Copy link
Collaborator

Just a note that I believe this will now always be the case unless you manually remove the secondary sidebar, because the searchbox.html template is included in the sidebar template list. This is expected to be empty, and is only used to place the "click to clear search highlights" button.

@tupui
Copy link
Contributor Author

tupui commented Jan 11, 2023

FYI I rebuild recently with main and the fix did not work anymore.

@choldgraf
Copy link
Collaborator

@tupui can you please provide more context than this? It is hard to understand what might be going wrong without at least a link to the configuration you're using and a reproducible example.

@tupui
Copy link
Contributor Author

tupui commented Jan 11, 2023

In the discussion we said that this was a workaround to the issue

html_theme_options = {
  ...
  "secondary_sidebar_items": ["page-toc"],
  ...
}

I am saying that it's not the case anymore. Compiling SciPy (cf the PR I linked in the description) still leads to the issue. Maybe @drammock can confirm using the same example he had?

Sorry I don't have more time to put into investigating doc issues at the moment. And tbh, I don't have the skills as I know close to nothing about internals of Sphinx. So it could take me unreasonable time to track down something.

@choldgraf
Copy link
Collaborator

I suspect that you've since upgraded to a newer version of this theme. The configuration value is now secondary_sidebar_items, check the docs here: https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/layout.html#secondary-sidebar-right

@tupui
Copy link
Contributor Author

tupui commented Jan 11, 2023

Yes I do have that, sorry for not updating the code e.g.

@choldgraf
Copy link
Collaborator

I think that this PR will fix the issue:

@tupui
Copy link
Contributor Author

tupui commented Jan 11, 2023

Great, thank you for having a look! Happy to test this when you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants