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

✨ ENH: Support captions in sidebar toctree #346

Merged
merged 10 commits into from
Mar 27, 2021

Conversation

jorisvandenbossche
Copy link
Member

Alternative for #135, now that we use the sphinx toc functions generated html more closely.

The main "problem" with how sphinx provides this functionality is that TocTree(app.env).get_toctree_for(pagename, ...) (which is returned in context["toctree"]()) has two aspects: 1) it always returns the full toctree and 2) it only inserts captions for the "first" level of the toctree.
So in our case, for the sidebar, we remove the first level of the toctree (which is in the navbar) and only keep the "current" subset, but this also means that with the default sphinx get_toctree_for we never get the captions for this "second" level of the toctree.

So how I tried to solve this is by making a "custom" version of get_toctree_for that creates the toctree for the relevant "index" page (the page that holds the .. toctree:: definition shown in the sidebar, which depends on the active page) instead of creating the toctree of the "master_doc" (typically the top-level index.rst, which always gives a document-wide toctree) as how it is hardcoded in sphinx' get_toctree_for.

This way, for the sidebar, we get the toctree of the index of the second level (eg demo/index.rst instead of index.rst), and then use the sphinx APIs to "resolve" that toctree as it would do otherwise, and thus also automatically inserts captions to the "first" level of that subset of the document-wide toctree, and thus we get sphinx to generate captions for the sidebar!

@jorisvandenbossche
Copy link
Member Author

And how this looks like on the demo site (with the current captions we already had in the sphinx docs, but which were not yet displayed):

image

@bollwyvl
Copy link
Collaborator

The feature is awesome... I wonder if the capitalization is perhaps too heavy. Perhaps it could be more blockquote-like, e.g. italic and indented?

@bollwyvl
Copy link
Collaborator

Oh, maybe it's a terminology thing: I was imaging figure and table captions, not toc captions... the capitalization may be appropriate, then, if that's more consistent.

@jorisvandenbossche
Copy link
Member Author

Yeah, indeed, this is about the sidebar toc ;)
The readthedocs theme uses uppercase captions (and the sphinx-book-theme, and furo, .. as well). Not directly sure about other themes (eg sphinx default doc theme doesn't use captions at all)

@@ -66,6 +66,7 @@
--pst-color-sidebar-link: 77, 77, 77;
--pst-color-sidebar-link-hover: var(--pst-color-active-navigation);
--pst-color-sidebar-link-active: var(--pst-color-active-navigation);
--pst-color-sidebar-caption: 77, 77, 77;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe re: uppercase, slam that into a variable as well?

--pst-sidebar-caption-transform: uppercase;
--pst-sidebar-font-size: 0.9em;

Copy link
Member Author

Choose a reason for hiding this comment

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

@bollwyvl sorry I didn't yet answer / address this comment. I am certainly fine with adding more styling knobs here, but maybe we can wait until someone needs it?

@@ -1,12 +1,17 @@
<nav aria-label="Main navigation" class="bd-links" id="bd-docs-nav">
<div class="bd-toc-item active">
<p class="caption">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, a few things from the accessibility angle:

  • the caption text could be put directly on the div, e.g. <div aria-label="Section 1"
  • there is also a dedicated HTML5 <caption> element, if it's really a caption

Copy link
Member Author

Choose a reason for hiding this comment

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

@bollwyvl the main problem here is that this html is generated by sphinx, so we don't have direct control over this snippet.

If we want to improve this, I think we either need 1) upstream changes to sphinx, or 2) we can adapt the beautifulsoup html object a bit to make some of those targetted changes (as we already do to add some additional classes for styling). But let's consider that out of scope for this PR.


soup = bs(toc_sphinx, "html.parser")

# # select the "active" subset of the navigation tree for the sidebar
Copy link
Collaborator

Choose a reason for hiding this comment

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

just making sure you notice this is still here :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the reminder ;) I still need to clean that up.

I am still a little bit wondering if my new code is fully equivalent in all cases to this beautifulsoup selector, but we will probably need to try out in practice to be sure ;)
(I checked locally with pandas and geopandas docs, and that seems all good)

@choldgraf
Copy link
Collaborator

@jorisvandenbossche I owe you a 🍺 for this one :-) TBH it all looks pretty good to me, just one small comment in there. The main thing I noticed is that we don't extend the tests by that much. Can we think of any edge-cases we may be missing? But if not, I think this is good to go 👍

@jorisvandenbossche
Copy link
Member Author

The main thing I noticed is that we don't extend the tests by that much. Can we think of any edge-cases we may be missing?

Yeah, I indeed only updated the existing tests (they already included the use of captions).
I can't directly think of more cases to test (but probably they will come up with bug reports .. ;)): we also already have a test that for the navbar a caption is not included, and we also have a test that the caption also works when having a single sidebar without navbar.

If you can think of something, happy to include more tests!

@bollwyvl
Copy link
Collaborator

Can we think of any edge-cases we may be missing?

It's a shame the sphinx build takes so long, as hypothesis could find our edge cases for us, but gets grumpy if a test excursion takes longer than a few 100ms.

We could go ahead and add codecov for basically free... I'll work up a PR...

@choldgraf
Copy link
Collaborator

any idea why codecov didn't show up here? 🤔

@bollwyvl
Copy link
Collaborator

bollwyvl commented Mar 27, 2021 via email

@choldgraf choldgraf closed this Mar 27, 2021
@choldgraf choldgraf reopened this Mar 27, 2021
@choldgraf
Copy link
Collaborator

Woohoo! Good call @bollwyvl . Let's merge this 🙂

@bollwyvl
Copy link
Collaborator

Not to be too finger-pointy, but this might be (the initial) culprit for #381.

@bollwyvl
Copy link
Collaborator

a quick band-aid for performance would be detecting lxml for bs4 calls (and making it an extra dependency)... it's also possible that some of the deep recursive calls could be replaced with xpath queries, which are much faster, as all the intermediate results wouldn't be created...

@jorisvandenbossche
Copy link
Member Author

My guess is that it is rather #349, and specifically the change of collapse=True to collapse=False (now, it's certainly possible that this PR also independently introduced some slowdown, which would then be exacerbated by #349 which increased the size of the toctree to process/resolve)

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 this pull request may close these issues.

3 participants