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: hide sidebar + navbar_align options #263

Merged
merged 14 commits into from
Nov 2, 2020

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Oct 6, 2020

This adds support for hiding sidebars on some page using html_sidebars. It does a few things to make this work:

  • Using html_sidebars, check for whether any sidebar items exist on a page, and if not, it shrinks and hides the sidebar.
  • Grows the content by 1-2 columns when there is no sidebar
  • Sets our navbar links to snap to the left so that no sidebars doesn't result in weird offsets

👆 That last one in particular may be worth discussing, but IMO it is a more robust solution than trying to make the navbar links line up with the page content. The reason I did this is because unless we keep the navbar items snapped to the left (or right) side of the page, any change in content width is going to be jarring for the user since they'll expect them to line up

And for our docs:

  • sets the search bar for our docs to be in the top-right so that it isn't hidden on a page w/ no sidebar.
  • sets the sidebar of the index page to hidden

Would love feedback on whether people think this is a good solution

New home page w/o sidebar on index:

image

Old home page with sidebar on index:

image

Partially addresses #262 #146

another option here would be to use a specific config like nosidebar for certain pages, rather than inferring it from the html_sidebars config (as @mwaskom suggested)...I'd be fine with that approach too but wanted to get this up here to look at

@choldgraf choldgraf changed the title sidebar shrink hide sidebar Oct 8, 2020
@choldgraf choldgraf changed the title hide sidebar ✨ NEW: hide sidebar Oct 8, 2020
@choldgraf
Copy link
Collaborator Author

choldgraf commented Oct 21, 2020

Can I please get some comments on this? Maybe either @hoetmaaiers or @jorisvandenbossche ? I just rebased and lost like 30 minutes trying to figure out how to re-build webpack and not generate more conflicts, so I would really like to either close this if people don't want it, or make modifications and get it merged so that I don't have to keep dealing with conflicts...

If folks are unhappy with the topbar behavior change, I can make it configurable for the user, but IMO it looks weirder to have the topbar mis-aligned from the text and in the middle if there is no sidebar, vs. just having the topbar items snap to the left all the time

(note, there are really only a few files changed here, the 66 files changed are because a bunch of packages updated when I ran the pre-commit)

@@ -12,7 +12,7 @@
<span class="navbar-toggler-icon"></span>
</button>

<div id="navbar-menu" class="col-lg-9 collapse navbar-collapse">
<div id="navbar-menu" class="col-11 collapse navbar-collapse">
Copy link
Collaborator Author

@choldgraf choldgraf Oct 21, 2020

Choose a reason for hiding this comment

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

here we make the topbar items position to the left, instead of being centered in the middle of the topbar

@@ -61,7 +66,7 @@
{% endblock %}

{% block docs_main %}
<main class="col-12 col-md-9 col-xl-7 py-md-5 pl-md-5 pr-md-4 bd-content" role="main">
<main class="col py-md-5 pl-md-5 pr-md-4 bd-content" role="main">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setting the main content to just col as I believe this will make it grow to fill up available space around it, rather than explicitly defining its size.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that this change is causing the problem on https://pydata-sphinx-theme--263.org.readthedocs.build/en/263/demo/demo.html
I suppose there is some content on that page that is larger, and causes the main text column to be wider if it is not constrained to a max size, and then causing the right toc sidebar to have no space anymore to appear on the side

@jorisvandenbossche
Copy link
Member

@choldgraf taking a look now (and thanks for working on this!)

Hmm, if the dev setup with webpack keeps giving you headaches, we need to reconsider it (I don't fully understand what went wrong in this case, for me it mostly works smoothly, but it seems you updated the versions of the vendored assets (fonts, fontawesome)).

Generally it looks like a nice and clean solution to have the "no-sidebar" option. I am not yet fully convinced about the alignment of the links in the navbar, so if we could leave that for a separate PR that might be good (or configurable? although that might be hard). Will try to give some more input about that on the issue.

Looking at the preview docs, I notice a few issues:

another option here would be to use a specific config like nosidebar for certain pages, rather than inferring it from the html_sidebars config

Do you know if it would be possible in sphinx to update this html_sidebars config when processing a given page based on such a nosidebar flag?
(certainly doesn't need to be done here, to be clear)

@jorisvandenbossche
Copy link
Member

Do you know if it would be possible in sphinx to update this html_sidebars config when processing a given page based on such a nosidebar flag?

I suppose that if we want a nosidebar flag to also work, it could be done by changing the check {% if sidebars %} in the jinja template to something like {% if sidebars and not (meta is not none and 'nosidebar' in meta)%}
(but it's also providing two ways to do the same thing .. so not necessarily needed)

@choldgraf
Copy link
Collaborator Author

choldgraf commented Oct 21, 2020

@jorisvandenbossche thanks for the review! A couple responses:

On pydata-sphinx-theme--263.org.readthedocs.build/en/263/demo/demo.html, the toc sidebar is messed up (it is shown in the left instead of right sidebar, fully at the bottom of the page). Not fully sure if it's a glitch with the preview, or actually caused by the changes in this PR (it also doesn't consistently happen on all pages)

good catch, I think this is actually related to using col in the main content. I will revert that and go back to explicitly defining the width.

I am not yet fully convinced about the alignment of the links in the navbar, so if we could leave that for a separate PR that might be good (or configurable? although that might be hard).

Lemme see how much work it'd be to make it configurable. It may not be too bad. Something like left_justify_navbar_links = False by default?

Due to the search box in the navbar, the height increased a bit (there is some padding around the searchbox div, that might need to be reduced for having it in the navbar). This also relates to #258 (which @hoetmaaiers is looking into)

Since that issue is not related to this PR and is more about the pydata theme docs themselves, how about I just move the search bar back to its normal position, and then I'll choose a page deeper in the docs where we can demo the "no sidebars" option rather than having it be the landing page. When we fix #258 then we can move the docs search bar to the navbar if we wish.

If the search box is not present, we might need to add some extra margin in the left sidebar (or in general add it, and remove some padding of the searchbox). For example here pydata-sphinx-theme--263.org.readthedocs.build/en/263/contributing.html we could use some more space above the content in the sidebar.

I will play around with this and try to get it to a nicer-looking default.

(but it's also providing two ways to do the same thing .. so not necessarily needed)

Yeah this was my feeling too. How about we start off with just using html_sidebars, since that's what most Sphinx sites use, and we can experiment with page-level metadata later on. That work?

@jorisvandenbossche
Copy link
Member

How about we start off with just using html_sidebars, since that's what most Sphinx sites use, and we can experiment with page-level metadata later on. That work?

Certainly!

@choldgraf
Copy link
Collaborator Author

@jorisvandenbossche take a look at the latest push and see if that fixes things up. I believe that I addressed the comments that you gave.

This is the new page w/ no sidebar: https://pydata-sphinx-theme--263.org.readthedocs.build/en/263/changelog.html

And just to confirm, this page now works: https://pydata-sphinx-theme--263.org.readthedocs.build/en/263/demo/demo.html

@choldgraf
Copy link
Collaborator Author

OK @jorisvandenbossche - I've applied the patch you recommended and it worked! Also had to rebase to avoid a merge conflict with the hashes. I think this is ready to go if you're 👍 on the changes.

@choldgraf
Copy link
Collaborator Author

I take it back! Need to figure out one bug on the sidebar width

@choldgraf
Copy link
Collaborator Author

OK fixed - now it's ready for review and/or merge. You can demo here: https://pydata-sphinx-theme--263.org.readthedocs.build/en/263/changelog.html

(sorry for the noise @jorisvandenbossche )

@choldgraf
Copy link
Collaborator Author

choldgraf commented Oct 31, 2020

@mwaskom it is how you configure the sidebars for any pages, including choosing particular components for the sidebar on subsets of pages, or removing it entirely in some cases.

html_sidebars is a core sphinx feature: https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_sidebars

so if you do:

html_sidebars = {
  "page1": ["thing1.html", "thing2.html"],
  "section1/*": ["thing2.html"],
  "page3": [],
}

then the default sidebar components will show up on all pages except page1, page3, and all sub-pages within section1/, while the components specified here would show up on page1 and section1/*, and page3 would have no sidebar.

@jorisvandenbossche
Copy link
Member

@choldgraf thanks for the updates! Playing around a bit with the demo docs looks good now

Lemme see how much work it'd be to make it configurable. It may not be too bad. Something like left_justify_navbar_links = False by default?

The options looks fine. I am only thinking about the type of value, which is now a boolean, and thus only leaves two options. Maybe something like navbar_alignment="left"|"content" is more robust if we also want to add a "right" option?

@choldgraf
Copy link
Collaborator Author

choldgraf commented Oct 31, 2020

Hmmm - I don't have strong opinions on bool vs. left/content...I'd be happy to go with that if you think it's better

@hoetmaaiers
Copy link
Collaborator

Other then the suggestion to rework navbar_alignment to a string for extensibility, I have no other remarks.

@choldgraf
Copy link
Collaborator Author

OK so taking the feedback of @jorisvandenbossche and @hoetmaaiers , the config is now called navbar_align and can take 3 strings: left, content, and right. I re-worked the Jinja logic to use a Python function so that it's not too confusing to understand.

Here's what navbar_align="right" looks like:

image

@mwaskom
Copy link

mwaskom commented Nov 1, 2020

Seemed pretty easy to get working, and did what I expected it to. The seaborn docs have some other aspects that render poorly with (at least the default config of) the pydata theme, but this one was a big hurdle. 👍

@choldgraf
Copy link
Collaborator Author

choldgraf commented Nov 1, 2020

@mwaskom I'm curious what are the other places where the pydata theme doesn't work properly! (maybe in a different issue?)

@mwaskom
Copy link

mwaskom commented Nov 1, 2020

Sure, but I feel like I should put in at least a minimum amount of effort to figure out whether it’s just a config setting that I can change :)

Seaborn docs also do a lot of bespoke html fiddling on top of the current Sphinx theme, and not at all in a principled or well-implemented way, so...

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

@jorisvandenbossche jorisvandenbossche changed the title ✨ NEW: hide sidebar ✨ NEW: hide sidebar + navbar_align options Nov 2, 2020
@jorisvandenbossche jorisvandenbossche merged commit edc2a0b into pydata:master Nov 2, 2020
@jorisvandenbossche
Copy link
Member

@mwaskom any additional feedback / feature requests to get it working as you want with seaborn is very welcome!

@choldgraf
Copy link
Collaborator Author

wohoo! 🎉

@CodeCox
Copy link
Contributor

CodeCox commented Dec 4, 2020

@choldgraf There seems to be a minor typo in the configuration documentation:

   "navbar_align_with_content": "left"

should read:

   "navbar_align": "left"

occurs 3x ie. this config key is shown incorrectly for each of the three values.

(Apologies if this comment is in the wrong place but this has not been officialy released yet therefore I did not want to raise an issue.)

@choldgraf
Copy link
Collaborator Author

ah yes I believe you are right @CodeCox ! Wanna make a docs PR to change it? I'm happy to review

@CodeCox
Copy link
Contributor

CodeCox commented Dec 5, 2020

@choldgraf Yes, I'm happy to give it a shot in the next few days!

It's a trivial change but I need to figure out the PR process for this repository. It will be good preporatory learning for me.

(The code for this PR has not yet been officially released but the related doc is already live - so there is already a disjoint because of this disparity!)

@choldgraf
Copy link
Collaborator Author

I don't think here's a giant problem, but we should definitely fix soonish. No hurries and I am happy to help if you hit issues 👍

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.

5 participants