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

✨ NEW: Collapsible lists in sidebars #226

Merged
merged 23 commits into from
Oct 16, 2020

Conversation

AakashGfude
Copy link
Member

@AakashGfude AakashGfude commented Oct 5, 2020

A minimalistic collapsible TOC heading feature which looks like:

Screen Shot 2020-10-05 at 11 02 31 am

The transition speed of opening and closing the panel is in the "slow" setting and the speed can be increased.

One abruptness in the whole experience is that since the clicking on a heading leads to the loading of the heading page, the panel does not open/close (due to obvious reasons). Which we can either leave as it is, or go beast mode and make this theme a single page application (probably not)

Tests have not been implemented yet. Will do it once the design/functionality is finalized.

cc: @choldgraf @akhmerov @pradyunsg

fixes #112

@pradyunsg
Copy link
Member

Ooooo, nice!

My only suggestion would be to make the "indicator" for collapse floated to the right -- like in Just the Docs.

@AakashGfude
Copy link
Member Author

Thanks, @pradyunsg for the suggestion. Have added the change. Lemme know what you think.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I like the look and 2 more suggestions:

  • The current "slow" speed is too slow, and I think a faster animation (or no animation) would be much better. The user is trying to navigate the tree, and is not here to watch animations. :P
  • The "chevron" feels too big and also gets hidden underneath the MacOS's hidden-unless-scrolling scrollbar. Inline suggestion to work around this below.

src/scss/_main.scss Outdated Show resolved Hide resolved
@choldgraf
Copy link
Member

This looks really nice! Thanks @AakashGfude for the work! In general I think this functionality is just what we want and I like the minimalist approach.

Agree with @pradyunsg's comments re: size and speed, I think some minor tweaks would improve things a bit

How do buttons like these get added to sections? e.g. I noticed that the sub-sub-sections don't have the button so just curious how the behavior should be controlled.

@AakashGfude
Copy link
Member Author

Thanks @pradyunsg for the detailes suggestions. will implement them.

  • The current "slow" speed is too slow, and I think a faster animation (or no animation) would be much better. The user is trying to navigate the tree, and is not here to watch animations. :P

I was doing it for the kids. but I guess I had the user base a bit wrong. 😅

How do buttons like these get added to sections? e.g. I noticed that the sub-sub-sections don't have the button so just curious how the behavior should be controlled.

@choldgraf at present the buttons are also assigned to li tags wit toctree-l1 as class. which is the first level of the lists. we can go deeper and assign it to toctree-l2 etc as well if need be.

@AakashGfude
Copy link
Member Author

AakashGfude commented Oct 5, 2020

You might even want to use 1.5em for the right margin, although the spacing when the scrollbar is not hidden/overlaid.

@pradyunsg I probably would not go for too much margin values, because it varies between screens dimensions. on my laptop. the latest margin value already does a lot of separation.

Screen Shot 2020-10-06 at 10 54 26 am

Mine is MacOS as well. but there seems to be an intrinsic margin for the scrollbar even when it's not visible. and the icon gets shifted left when the scrollbar appears.

@chrisjsewell
Copy link
Member

Looks good cheers @AakashGfude
I'll play around properly with it at some point, but off the top of my head, have you checked how it degrades when JavaScript is disabled?

@AakashGfude
Copy link
Member Author

AakashGfude commented Oct 6, 2020

I'll play around properly with it at some point, but off the top of my head, have you checked how it degrades when JavaScript is disabled?

@chrisjsewell the opening/closing of lists don't work. and the sidebar goes back to functioning like before. the chevron icons are still there though.

@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 6, 2020

thanks for looking, yeh the main by is obviously that the site should still be useable if JavaScript is disabled, but it sounds like that is not an issue

So, I've had a look now. A few issues:

  1. The top-level section titles should also be collapsible, i.e. Main Docs and Reference Items in the documentation
  2. There is no nested collapsible sections, like in the just-the-docs documents. The nesting level of collapsible headings could perhaps be a configuration variable (in theme.conf)
  3. As you mentioned, currently the collapsible sections are also the URL links, so exposing a section means you have to navigate to it. That has to be fixed before merging.
    There are maybe two solutions to this; make only the chevron itself clickable to toggle the collapse, as is done in the just-the-docs example, or make collapsible titles separate to URL links, like is done in my other gold standard for these collapsible ToCs; the VS Code documentation: https://code.visualstudio.com/docs

@AakashGfude
Copy link
Member Author

AakashGfude commented Oct 6, 2020

Aweome @chrisjsewell . great points.

  1. The top-level section titles should also be collapsible, i.e. Main Docs and Reference Items in the documentation.

This sounds good.

  1. There is no nested collapsible sections, like in the just-the-docs documents. The nesting level of collapsible headings could perhaps be a configuration variable (in theme.conf)

We should definitely handle all expandable sections. not sure if it should be in theme.conf though. As this extra config may not be that useful or used. Everyone would probably just want all of their sections to be collapsible?

There are maybe two solutions to this; make only the chevron itself clickable to toggle the collapse, as is done in the just-the-docs example, or make collapsible titles not URL links, like is done in my gold standard for these collapsible ToCs; the VS Code documentation: https://code.visualstudio.com/docs

From the looks of it doing what just-the-docs does atm, won't change anything in comparison to present implementation from the user experience point of view? i.e., just-the-docs also navigates to the section and opens it up.
For VS code type implementation, we will then have to make an extra entry for the title URL as well in the section ?

Our navigation through the sidebar makes the whole sidebar jittery on a new page load, which makes the experience quite unpleasant in comparison to just-the-docs. We should probably fix this jitteriness first and then see?

@chrisjsewell
Copy link
Member

just-the-docs also navigates to the section and opens it up.

No it doesn't, not if you click directly on the chevron

@chrisjsewell
Copy link
Member

In just-the-docs, the chevrons are completely seperate (from the text), clickable SVGs, rather than fontawesome icons

@AakashGfude
Copy link
Member Author

AakashGfude commented Oct 6, 2020

ahh yeah. did not click directly in the chevron to open up a different section. my bad.

but clicking on a title to navigate to a different section is a bit unpleasant though 😬 . Which we can probably target as a separate issue in a later release :)

@AakashGfude
Copy link
Member Author

have made only the icons clickable like just-in-docs now. for everyone to try and give feedback on.

@chrisjsewell
Copy link
Member

The feedback is that it doesn't look lol. Your chevrons are still underneath the links

@AakashGfude
Copy link
Member Author

The feedback is that it doesn't look lol. Your chevrons are still underneath the links

Oops haha. Sorry, in transit. Will reach a stable place and push the rearrangement of elements edit. Sorry for giving false hope.

@AakashGfude
Copy link
Member Author

@chrisjsewell should be in a working state now.

@chrisjsewell
Copy link
Member

Yep the chevron only collapsing works now thanks. I wonder if we should make that more intuitive though? Something like the cursor icon changes when you hover over it?

@AakashGfude
Copy link
Member Author

Yep the chevron only collapsing works now thanks. I wonder if we should make that more intuitive though? Something like the cursor icon changes when you hover over it?

That's a good idea. some ways can be 1) increase the font size on hover 2) change color 3) change background-color

@AakashGfude
Copy link
Member Author

AakashGfude commented Oct 8, 2020

TODO:

  • Add possible tests - HTML generated is already tested against.
  • Make this PR mergeable

@AakashGfude AakashGfude marked this pull request as ready for review October 8, 2020 12:33
@AakashGfude
Copy link
Member Author

Have added color change when hovering on icons. Icon size increase and the border getting colored didn't look right. ideas on anything more dramatic are welcome.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

If we're making the collapsing chevrons recursive, I think we should also start with the deeper levels collapsed by default.

src/scss/_main.scss Outdated Show resolved Hide resolved
@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 8, 2020

Looking better 😄

I would definitely like to control the initial collapsed level though (via a config value), i.e. I think for the documentation here, this is how the ToC should initialise:

image

this would be something like toc_depth=1, with toc_depth=0 being that the top-level headings are also collapsed.

@chrisjsewell
Copy link
Member

I would definitely like to control the initial collapsed level though (via a config value)

Oh I just noticed this is similar to what @pradyunsg said, but yeh it should be configurable

@choldgraf
Copy link
Member

choldgraf commented Oct 8, 2020

I think this looks really nice! Two quick thoughts:

  • Agree about default collapse configuration. The pydata theme calls this show_toc_depth (ref), maybe we can call it show_navbar_depth?
  • What do folks think about not adding a collapse button to part headers? The contents of these headers will always be shown by default (since that's the top-level of the TOC), so the button would only be used to close the section but not open it. I feel like this feature is more useful for folks that want to browse hidden sections without changing pages, and the extra button for each "part" header (which is unclickable anyway) adds some visual clutter

@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 8, 2020

The contents of these headers will always be shown by default

Why should these alway be shown by default?
They're not in https://code.visualstudio.com/docs, which has "non-link" top-level headers.
In fact, like there, it would be ideal if these headers were actually "fully clickable" (as opposed to only the chevrons for the "link" headers).

@choldgraf
Copy link
Member

I'm making the assumption:

  • Those headers are not clickable (since Sphinx doesn't make them clickable, though I guess we can over-ride Sphinx)
  • Those headers don't point to a specific page, they are just a word describing a collection of pages

I agree that in theory they could be clickable, we change how Sphinx handles the relationship between :caption: and toctree headers, but at least thus far the more common use-case has been to just have them as headers with shown links below, and I think we should be designing for the more common use-case at least initially. We can always add the ability to collapse those sections later on, or add a config value to make it possible.

@choldgraf
Copy link
Member

choldgraf commented Oct 8, 2020

Also just a note - I wouldn't have a problem with this if the caret only showed up upon hover. If that was the case then I wouldn't mind one way or another - mostly I think it's just too visually-cluttered to have all the carets at once (at least at the top level)

(this specific grouping of links is what made me think this)

image

@chrisjsewell
Copy link
Member

oh yeh just pre-approving, assuming the docs will be updated

@AakashGfude
Copy link
Member Author

Yes, in the process of writing the documentation.

@AakashGfude
Copy link
Member Author

Have written documentation but not sure how clear it is. Someone with better writing skills, please check 😬 .

@chrisjsewell
Copy link
Member

Yep looks fine to me. Let's get this show on the road!

@choldgraf
Copy link
Member

One question - I noticed that the default depth is 2. Which means that by default we'll show top-level chapters and each of their nested sections (but we won't show sub-sections). That deviates a bit from the typical Sphinx theme behavior, which only shows top-level behavior, right?

My intuition is that we don't want the SBT to behave that different from most other Sphinx sites. What do you all think about setting the default level to 1?

@chrisjsewell
Copy link
Member

Is this not also in some way controlled by the :maxdepth: option of the toctrees: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-toctree? I guess that sets what is "available" in the fully expanded ToC

@choldgraf
Copy link
Member

Ah - I always thought that :maxdepth: only applied to the inserted TOCTree, not the one in any sidebars...

@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 15, 2020

Yeh not sure, never really thought about it TBH

@AakashGfude
Copy link
Member Author

:maxdepth: does not change anything about the toctree object we capture using bs4. which is what we use to render the sidebar.

@AakashGfude
Copy link
Member Author

AakashGfude commented Oct 15, 2020

:maxdepth: does not change anything about the toctree object we capture using bs4. which is what we use to render the sidebar.

Also because we are explicitly specifying the maxdepth value as -1 while capturing it .

# Grab the raw toctree object and structure it so we can manipulate it
        toc_sphinx = context["toctree"](
            maxdepth=-1, collapse=False, titles_only=True, includehidden=True
        )
        toctree = bs(toc_sphinx, "html.parser")

And I reckon we want this, otherwise, we won't be able to show sections(expand) at all, as it won't be there in the generated HTML itself. If the max depth were, for example, 1

@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 15, 2020

:maxdepth: does not change anything about the toctree object we capture using bs4. which is what we use to render the sidebar.

I see, I guess the question is, should there ideally be any integrations with :maxdepth: (and the other hard-coded options)?
Is this taking away any of sphinx's functionality from the user?
(probably not, but I think its worth considering)

@AakashGfude
Copy link
Member Author

I see, I guess the question is, should there ideally be any integrations with :maxdepth: (and the other hard-coded options)?
Is this taking away any of sphinx's functionality from the user?

I think we can utilize maxdepth value to set the cap on the levels to include in the sidebar. and then configure on that sidebar which levels should be expanded/collapsed using show_sidebar_depth?
But probably sidebars are meant to include all levels?

@AakashGfude
Copy link
Member Author

Have made the default level as 1 now.

@chrisjsewell
Copy link
Member

But probably sidebars are meant to include all levels?

For sure its more important when you don't have access to these collapsible ToCs

But, I guess for maximum configuration, it would be ideal to be able to say "under this section I want everything collapsed, and for this section I want to uncollapse one level, ...", i.e. it would be used like a per-toctree show_sidebar_depth.
But not saying that needs to be implemented in this PR

@choldgraf
Copy link
Member

it would be ideal to be able to say "under this section I want everything collapsed, and for this section I want to uncollapse one level, ...", i.e. it would be used like a per-toctree show_sidebar_depth.

I'm +1 on this as well (assuming this is how Sphinx expects us to use maxdepth). I think it's best as a new issue so that we can use this nice new feature ✨

@choldgraf
Copy link
Member

ah - just noticed a problem. If I'm on a page then its children are not collapsed:

image

e.g.: https://sphinx-book-theme--226.org.readthedocs.build/en/226/reference/subchapter1b/index.html#

when I click that link, the sub-pages underneath do not show up

@AakashGfude
Copy link
Member Author

when I click that link, the sub-pages underneath do not show up

ooh yeah, grrr, forgot about this feature. :(

@AakashGfude
Copy link
Member Author

@choldgraf, Have added the feature to expand immediate child ul as well on clicking a link, or page load. let me know how it feels.

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

I think it looks great!

@choldgraf choldgraf merged commit 46ce4b7 into executablebooks:master Oct 16, 2020
@choldgraf
Copy link
Member

alright! Features in, tests happy, docs written, thumbs up given, let's merge it in and start playing around with it! 🎉

Thanks @AakashGfude for the improvement!

@chrisjsewell
Copy link
Member

cheers!

@choldgraf
Copy link
Member

I kinda wanna cut a little release...@chrisjsewell did the sidebars update give you any trouble? I wanted to make sure we didn't introduce any bugs when that one got merged.

@choldgraf
Copy link
Member

they look pretty good to me!

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.

Collapsible ToC headings
4 participants