ENH: Left navigation menu to collapse at the "part" level

[AakashGfude]

The following PR adds the ability to make parts collapsible as well like chapters. It adds a chevron next to the caption and makes the caption text and chevron clickable.

EDIT This functionality is only added at present when show_nav_level: 0.

Screen.Recording.2022-02-25.at.9.24.43.am.mov

Had to restructure the HTML, so that the caption p tag and the corresponding ul containing chapters for that part are encapsulated in a parent div. This encapsulated structure was followed by all the other collapsible nav elements. The CSS, and other code were written taking that into account.
Started off with not altering the HTML and doing a workaround for the part level. But was turning out to be hackier and hackier as I progressed.

Have altered the HTML using beautifulsoup, as the HTML I think is directly generated by sphinx.

• restructure HTML to make the sidebar a proper ul list, to accomodate this PR's feature.
• rewrite some CSS to adjust nav to the new HTML structure
• write CSS for collapsing parts
• correct tests

fixes executablebooks/sphinx-book-theme#460

[choldgraf]

Hmmm - I am not sure why you need to re-work the HTML generation here to wrap with div s. Here is the current "caption / list" structure:

<p class="caption">Caption 1</p>
<ul> ... TOC links

<p class="caption">Caption 2</p>
<ul> ... TOC links

Couldn't we just use the same label/input pattern like this:

<p class="caption">Caption 1</p>
<label for="captioninput1">
<input class="toc-toggle--caption" id="captioninput1">
<ul> ... TOC links

<p class="caption">Caption 2</p>
<label for="captioninput2">
<input "toc-toggle--caption" id="captioninput2">
<ul> ... TOC links

And then define a CSS selector like:

input.toc-toggle--caption:checked + ul {
    display: none;
}

?

[AakashGfude]

@choldgraf I could not use that HTML structure because I needed to position the chevron in the label next to the p caption tag. And it is not possible to do that for every p tag as they are at the same level.

So, I had used something like:

<input class="toctree-checkbox" id="toctree-checkbox-2" name="toctree-checkbox-2" type="checkbox">
<p class="caption">
 <span class="caption-text">
  Reference
 </span>
 <label for="toctree-checkbox-2">
  <i class="fas fa-chevron-down">
  </i>
 </label>
</p>

which puts the label inside the p tag, for easy positioning.
But then I had to adjust the code here https://github.com/pydata/pydata-sphinx-theme/blob/master/src/pydata_sphinx_theme/__init__.py#L138, as it had a different structure. And also putting labels inside the p tag did not feel like the best HTML.

And also I had to write separate CSS for the part and for the rest of the collapsible elements as they had different structures.

[AakashGfude]

Stuck in an error. When I set show_nav_level 0. The parts are collapsed, which is desired. When I navigate to a children's page by opening the chevrons. Once the page loads, the parts get collapsed again. Seems like the checkbox of parts always get unchecked when reloading. Happens only with show_nav_level 0, does not happen with numbers above that. Probably has to do something with https://github.com/pydata/pydata-sphinx-theme/blob/master/src/pydata_sphinx_theme/__init__.py#L138

[choldgraf]

Left a few comments - this looks like a good start! I am a bit worried we're adding unnecessary complexity by re-working the whole TOC with a new list level when we want captions to be collapsed.

What if we only wrapped the p.caption element in a label, so that the whole thing were clickable:

<input ...>
<label><p class="caption">Part caption</p><i ... ></label>
<ul ...>

Then the icon wouldn't be inside of the <p> tag, but could still be positioned/styled in a sensible way via the surrounding label block. This could be styled to suggest the whole thing is clickable. e.g. here's how it looks in the microsoft docs):

[choldgraf]

@AakashGfude I believe that the logic for automatically opening checkboxes for an active page is actually here:

pydata-sphinx-theme/src/pydata_sphinx_theme/__init__.py

Lines 295 to 298 in c987276

	# if this has a "current" class, be expanded by default
	# (by checking the checkbox)
	if "current" in classes:
	checkbox.attrs["checked"] = ""

So you'd want to add a similar piece of logic to that block that also detects a caption that corresponds to that section of the toctree, and opens it.

[AakashGfude]

Thanks @choldgraf , I had to add the logic to expand the part explicitly here 6698203#diff-d9f80958bbaaf03124bb877a07f31183c094105bbe78ea167de87e02b18d66b7R284, as it does not seem to have the current class when show_nav_level: 0. Which makes it work now.
Not sure where is the logic to add current class to an element, but will try to find it.

[choldgraf]

I believe that current is added by Sphinx, so perhaps after you've nested everything you can do something like

if new_soup.select(".current"):
  # Check the top `ul`

[AakashGfude]

@choldgraf, have implemented similar logic. if the child li elements have current class then "checking" the parent part

[AakashGfude]

I am a bit worried we're adding unnecessary complexity by re-working the whole TOC with a new list level when we want captions to be collapsed.

What if we only wrapped the p.caption element in a label, so that the whole thing were clickable:

<input ...>
<label><p class="caption">Part caption</p><i ... ></label>
<ul ...>

Then the icon wouldn't be inside of the <p> tag, but could still be positioned/styled in a sensible way via the surrounding label block. This could be styled to suggest the whole thing is clickable:

@choldgraf This works and sounds like a reasonable solution. But, we will still need to write custom scss for captions separately. For example ~ is used everywhere, which wont work here:

.toctree-checkbox {
  ~ ul {
    display: none;
  }

Also, need to write custom code in _add_collapse_checkboxes specifically for captions, as it is not an li. And possibly in a few other places. Although we are writing some extra code now as well, I feel like the structure is consistent with the rest of the child elements. Like toctree-l{i} is a class for li, but we will have to assign toctree-l0 to some other tag. Overall, I feel like, since the sidebar functions like a list the whole thing makes more sense semantically to be wrapped in one.
Parts are structurally separated as each part has its own parent element (li in our case). Otherwise, the whole thing was just based on the ordering of elements, in the parts level.
Feels more robust as well.

What do you think?

[choldgraf]

I think that's a reasonable rationale - I had also mistakenly thought you were bumping the nesting level of all the other list items (so l1 became l2 etc), but I see you're just adding an l0 which makes sense.

What do you think about making the whole p.caption clickable though?

[AakashGfude]

What do you think about making the whole p.caption clickable though?

I will give it a shot, using your idea. I think it's a useful addition.

[AakashGfude]

• making the part text clickable.

[AakashGfude]

• proper functionality of show_nav_level

[AakashGfude]

@choldgraf Should we do collapse_parts ? as stated in executablebooks/sphinx-book-theme#460. There's already a collapse_navigation option as well.

[choldgraf]

A few quick thoughts:

• re: collapse_parts. I don't think we should use that name, because collapse_navigation behaves very differently - it totally removes the ability to expand the navigation links (https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/configuring.html#remove-reveal-buttons-for-sidebar-items).

To start with, maybe we trigger this behavior only if what is the alternative? To infer whether parts should be collapsed based off of whether show_nav_level=0? And if users ask for the ability to both make parts collapsible and want some of them shown by default, then we can add the functionality later. What do you think?

Also please add documentation to this PR - it makes it easier for reviewers to understand what functionality you're adding and what the API looks like!

[AakashGfude]

To start with, maybe we trigger this behavior only if what is the alternative? To infer whether parts should be collapsed based off of whether show_nav_level=0? And if users ask for the ability to both make parts collapsible and want some of them shown by default, then we can add the functionality later. What do you think?

@choldgraf sounds good.

Also please add documentation to this PR - it makes it easier for reviewers to understand what functionality you're adding and what the API looks like!

Have edited the top description and also added a bit in the doc.

[AakashGfude]

• parts are collapsible only when show_nav_level: 0. Else there is no chevron the caption.

[AakashGfude]

• test for show_nav_level : 0

[AakashGfude]

@choldgraf show_nav_level: 0 will only trigger this PR's functionality now. Have added a bit of docs, and tests as well.

[choldgraf]

Thanks for working on this @AakashGfude - a few more thoughts and suggestions here!

Also one other thing - the UX of the collapsing is a little bit weird. I can click only the top line of a caption if it spans multiple lines, but the other lines are just "regular" text. What's going on there?

[choldgraf]

I'm confused, why would the navigation depth effect column alignment and margin?

[choldgraf]

Instead of doing full regression checks for each of these, why not test specifically for the things that you want to be the case. I think that will reduce the extra cruft in the regression tests and make this test more explicit. So:

• Test that the toctree has a top-level ul with a class list-caption
• That the next li has toctree-l0 has-children
• Some basic checks that everything inside has the same structure as we'd expect, but don't need to go crazy here.

[AakashGfude]

@choldgraf, have altered the tests to check for only specific parts. Is it clearer?

[AakashGfude]

Also one other thing - the UX of the collapsing is a little bit weird. I can click only the top line of a caption if it spans multiple lines, but the other lines are just "regular" text. What's going on there?

Have corrected the CSS now. 4650f39

[choldgraf]

This is looking pretty good to me! I tried it locally and the weird CSS behavior I mentioned is now gone!

I see that @damianavila wants to take a look as well, so let's see what he thinks.

[damianavila]

Maybe we can showcase this functionality on our demo site or do you think is too much, @choldgraf?

[damianavila]

This is nice, @AakashGfude!
Btw, code LGTM, but left some comments/questions about docs and showcase of the feature 😉.

[choldgraf]

I added a quick commit to try and drive more attention to the "caption" terminology, rather than "parts", because I think that caption is easier to tie directly to the toctree directive, and is a more common use. I also tried to clarify the instructions a bit.

I agree it'd be nice to demonstrate this, but I kind-of feel like we don't want to collapse all of our toctrees at the "part" level. It's too bad that we can't trigger this behavior only for certain pages or something, but maybe we can add a demo or screenshot in the future if the functionality is confusing? I think I'm +1 on merging as-is.

[choldgraf]

I think that @damianavila was +1 on this as well, so with the latest docs update I think we are good to go. Thanks for the improvement @AakashGfude 🙂