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

AakashGfude · 2022-02-22T00:11:04Z

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 · 2022-02-22T00:18:56Z

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 · 2022-02-22T00:41:10Z

@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 · 2022-02-22T12:21:54Z

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  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 · 2022-02-22T14:13:04Z

src/pydata_sphinx_theme/__init__.py

+            ii = 0
+            while True:
+                ii += 1
+                for checkbox in new_soup.select(
                    f"li.toctree-l{ii} > input.toctree-checkbox"
                ):
                    checkbox.attrs["checked"] = None


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

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.

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`

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

src/pydata_sphinx_theme/assets/styles/index.scss

src/pydata_sphinx_theme/__init__.py

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

…a-sphinx-theme into parts-collapse

AakashGfude · 2022-02-23T00:35:42Z

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>Part caption</label>
<ul ...>
Then the icon wouldn't be inside of the  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 · 2022-02-23T04:03:55Z

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 · 2022-02-23T04:28:02Z

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 · 2022-02-23T06:15:51Z

making the part text clickable.

AakashGfude · 2022-02-23T06:19:00Z

proper functionality of show_nav_level

AakashGfude · 2022-02-23T06:46:17Z

@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 · 2022-02-24T23:07:13Z

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 · 2022-02-24T23:58:57Z

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

AakashGfude · 2022-02-25T00:44:16Z

test for show_nav_level : 0

AakashGfude · 2022-02-25T01:27:58Z

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

docs/user_guide/configuring.rst

src/pydata_sphinx_theme/__init__.py

tests/test_build.py

choldgraf · 2022-02-27T14:05:43Z

tests/test_build.py

+    confoverrides = {"html_theme_options.show_nav_level": 0}
+    sphinx_build = sphinx_build_factory("sidebars", confoverrides=confoverrides).build()
+
+    # Both the column alignment and the margin should be changed


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

tests/test_build.py

choldgraf · 2022-02-27T14:09:54Z

tests/test_build.py

+    # Both the column alignment and the margin should be changed
+    index_html = sphinx_build.html_tree("section1/index.html")
+    sidebar = index_html.select("nav#bd-docs-nav")[0]
+    file_regression.check(sidebar.prettify(), extension=".html")


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.

tests/test_build.py

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

AakashGfude · 2022-02-28T01:49:19Z

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

AakashGfude · 2022-02-28T01:51:58Z

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 · 2022-03-01T04:00:57Z

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.

docs/user_guide/configuring.rst

damianavila · 2022-03-08T15:17:05Z

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

damianavila · 2022-03-08T15:18:57Z

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

docs/user_guide/configuring.rst

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 · 2022-03-11T06:31:30Z

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 🙂

restructuring html for parts

47985bb

AakashGfude added 4 commits February 22, 2022 14:06

added styling and restructured bs html

cbcc5c3

adding list-caption class

c33f15e

not adding collapse parts for now

c267479

modified tests

fc53df7

AakashGfude marked this pull request as ready for review February 22, 2022 12:25

pre-commit

c54dfb4

choldgraf reviewed Feb 22, 2022

View reviewed changes

AakashGfude and others added 3 commits February 23, 2022 11:06

Update src/pydata_sphinx_theme/__init__.py

dcf9c2c

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

opening captions explicitly

6698203

Merge branch 'parts-collapse' of https://github.com/AakashGfude/pydat…

f9a127b

…a-sphinx-theme into parts-collapse

AakashGfude added 3 commits February 23, 2022 16:45

making parts text clickable

3761169

style for part label

ef87163

pre-commit

67c4c2f

corrections in __init__ and tests

3785a47

choldgraf reviewed Feb 24, 2022

View reviewed changes

adding docs for show_nav_levek: 0

cce1bf9

parts collapse only when show_nav_level: 0

63acd91

tests update

cd7ecb3

AakashGfude added 2 commits February 25, 2022 12:08

added test for sidebar nav_level0

d67480e

docs edit

270fe93

choldgraf reviewed Feb 27, 2022

View reviewed changes

AakashGfude and others added 8 commits February 28, 2022 11:03

parts chevron style

4650f39

Update docs/user_guide/configuring.rst

99b2340

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

Update src/pydata_sphinx_theme/__init__.py

dca2f5d

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

Update src/pydata_sphinx_theme/__init__.py

c5ef7d1

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

Update src/pydata_sphinx_theme/__init__.py

8e94faf

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

Update src/pydata_sphinx_theme/__init__.py

a60b2a7

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

Update src/pydata_sphinx_theme/__init__.py

819caed

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>

tests and some code refactoring

2b43a8c

damianavila self-requested a review February 28, 2022 01:46

damianavila reviewed Mar 8, 2022

View reviewed changes

docs/user_guide/configuring.rst Outdated Show resolved Hide resolved

Clarify this in the docs

df2ca97

choldgraf reviewed Mar 10, 2022

View reviewed changes

docs/user_guide/configuring.rst Outdated Show resolved Hide resolved

Update docs/user_guide/configuring.rst

7da634d

choldgraf approved these changes Mar 10, 2022

View reviewed changes

Cleaning up docs

a3d4caf

choldgraf merged commit 6711e9b into pydata:master Mar 11, 2022

choldgraf mentioned this pull request Mar 25, 2022

RELEASE: v0.8.1 #611

Merged

gabalafou mentioned this pull request Jan 24, 2024

Make TOC sections expandable and collapsible by keyboard #1582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AakashGfude commented Feb 22, 2022 •

edited

choldgraf commented Feb 22, 2022 •

edited

AakashGfude commented Feb 22, 2022 •

edited

AakashGfude commented Feb 22, 2022

choldgraf left a comment •

edited

choldgraf Feb 22, 2022

AakashGfude Feb 23, 2022 •

edited

choldgraf Feb 23, 2022

AakashGfude Feb 23, 2022

AakashGfude commented Feb 23, 2022 •

edited

choldgraf commented Feb 23, 2022

AakashGfude commented Feb 23, 2022

AakashGfude commented Feb 23, 2022 •

edited

AakashGfude commented Feb 23, 2022 •

edited

AakashGfude commented Feb 23, 2022

choldgraf left a comment •

edited

AakashGfude commented Feb 24, 2022

AakashGfude commented Feb 24, 2022 •

edited

AakashGfude commented Feb 25, 2022 •

edited

AakashGfude commented Feb 25, 2022

choldgraf left a comment

choldgraf Feb 27, 2022

choldgraf Feb 27, 2022

AakashGfude commented Feb 28, 2022

AakashGfude commented Feb 28, 2022 •

edited

choldgraf commented Mar 1, 2022

damianavila commented Mar 8, 2022

damianavila commented Mar 8, 2022 •

edited

choldgraf left a comment

choldgraf commented Mar 11, 2022

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

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

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

Conversation

AakashGfude commented Feb 22, 2022 • edited

choldgraf commented Feb 22, 2022 • edited

AakashGfude commented Feb 22, 2022 • edited

AakashGfude commented Feb 22, 2022

choldgraf left a comment • edited

Choose a reason for hiding this comment

choldgraf Feb 22, 2022

Choose a reason for hiding this comment

AakashGfude Feb 23, 2022 • edited

Choose a reason for hiding this comment

choldgraf Feb 23, 2022

Choose a reason for hiding this comment

AakashGfude Feb 23, 2022

Choose a reason for hiding this comment

AakashGfude commented Feb 23, 2022 • edited

choldgraf commented Feb 23, 2022

AakashGfude commented Feb 23, 2022

AakashGfude commented Feb 23, 2022 • edited

AakashGfude commented Feb 23, 2022 • edited

AakashGfude commented Feb 23, 2022

choldgraf left a comment • edited

Choose a reason for hiding this comment

AakashGfude commented Feb 24, 2022

AakashGfude commented Feb 24, 2022 • edited

AakashGfude commented Feb 25, 2022 • edited

AakashGfude commented Feb 25, 2022

choldgraf left a comment

Choose a reason for hiding this comment

choldgraf Feb 27, 2022

Choose a reason for hiding this comment

choldgraf Feb 27, 2022

Choose a reason for hiding this comment

AakashGfude commented Feb 28, 2022

AakashGfude commented Feb 28, 2022 • edited

choldgraf commented Mar 1, 2022

damianavila commented Mar 8, 2022

damianavila commented Mar 8, 2022 • edited

choldgraf left a comment

Choose a reason for hiding this comment

choldgraf commented Mar 11, 2022

AakashGfude commented Feb 22, 2022 •

edited

choldgraf commented Feb 22, 2022 •

edited

AakashGfude commented Feb 22, 2022 •

edited

choldgraf left a comment •

edited

AakashGfude Feb 23, 2022 •

edited

AakashGfude commented Feb 23, 2022 •

edited

AakashGfude commented Feb 23, 2022 •

edited

AakashGfude commented Feb 23, 2022 •

edited

choldgraf left a comment •

edited

AakashGfude commented Feb 24, 2022 •

edited

AakashGfude commented Feb 25, 2022 •

edited

AakashGfude commented Feb 28, 2022 •

edited

damianavila commented Mar 8, 2022 •

edited