MAINT: Remove duplication with pydata-sphinx-theme

[choldgraf]

This is a pass at removing as much as we can from this theme in order to inherit more structure, style, and functionality from the PyData theme. It does not port over all the features of the pydata theme, but tries to remove complexity here so that it's easier to port them over in the future.

Note that this depends on the main branch of the PyData theme: you'll need to install that to get this to build properly until a release is made (should happen soon). I'm temporarily installing it in the .tox file so that it's easy to test locally.

I'd like to hand this off to @AakashGfude, who should feel welcome to either take it over and create his own PR, or to push directly to this PR and ask me for review as well.

To do

• [x] Make changes to support Bootstrap 5 (ref: https://github.com/pydata/pydata-sphinx-theme/pull/1029)
• [x] See if there's anything else we couple possibly remove from this theme that is supported in the PST
  • [x] Try removing all of our button / menu CSS and re-using the PyData / Bootstrap rules
• [x] Fix up our CSS in order to make the end-user UI behavior behave the same way as before
  • [x] The right table of contents button in the article header should only show on mobile screens
  • [x] Ensure that behavior showing/hiding the table of contents w/ margin content present is correct
• [x] Look at our reference/examples docs to confirm we have not introduced any bugs
• [ ] Final pass at the preview docs to make sure they look OK
• [x] Remove the git+ section of our dependencies so we aren't installing from main
• [ ] Merge

Few things I had to add back because they are not yet in pydata:

• CSS for footer-content, as only the section html is introduced in pydata without any styling.
• toc button show/hide styling in header-content, as pydata has that button in header instead.

References

• closes https://github.com/executablebooks/sphinx-book-theme/issues/630
• closes https://github.com/executablebooks/sphinx-book-theme/issues/648
• closes https://github.com/executablebooks/sphinx-book-theme/issues/545

[AakashGfude]

Listing out the features which we have removed here, and instead inheriting what pydata has(we can keep it as well, if people are keen):

• rtd button is no longer beneath the sidebar in a fixed position. Instead it is like pydata on the right bottom end of the page.
• the tooltips hover on header is like sidebar.

[choldgraf]

Just a quick note that:

rtd button is no longer beneath the sidebar in a fixed position. Instead it is like pydata on the right bottom end of the page

I think is actually because of the announcement banner on the top. I believe if that banner is gone, then it always exists at the bottom of the screen but double check that this is actually true :-)

Also the PyData v0.12 release candidate is out so I think you can update the install dependencies accordingly in this PR too: https://github.com/pydata/pydata-sphinx-theme/releases/tag/v0.12.0rc1

[choldgraf]

FYI for @AakashGfude - the PyData theme updated its HTML and JS to support Bootstrap 5 instead of Bootstrap 4. Since we're going to introduce a breaking change anyway, I suggest that we upgrade our own HTML, CSS, and JS to support these changes.

• https://github.com/pydata/pydata-sphinx-theme/pull/1029

I've added a todo item in the top comment, but let me know if you think this is better in a different PR.

[AakashGfude]

I've added a to-do item in the top comment, but let me know if you think this is better in a different PR.

@choldgraf maybe we should do it in a separate PR, as an sbt release holds up jupyter-book release. And these changes will slow that down further.

[AakashGfude]

Few things I had to add back because they are not yet in pydata:

• CSS for footer-content, as only the section html is introduced in pydata without any styling.
• toc button show/hide styling in header-content, as pydata has that button in header instead.

[AakashGfude]

Also, the main content now expands to occupy the space of right toc if there is no in-page toc present (default behaviour of pydata? ). Which I think looks nice, and does not need any fixing. What do people think?

[choldgraf]

maybe we should do it in a separate PR, as an sbt release holds up jupyter-book release. And these changes will slow that down further.

Sounds good - I think that makes sense as well...just will need to be explicit about the second breaking change in the next release. I've created an issue to track that below, and will remove the todo item from this PR:

• https://github.com/executablebooks/sphinx-book-theme/issues/648

Few things I had to add back because they are not yet in pydata:

Sounds good - can you add a tag or a comment for the things that we think could be upstreamed in the future, so we know where we'd like to continue removing functionality and pushing it upstream?

the main content now expands to occupy the space of right toc if there is no in-page toc present

The only problem with this is margin content. If the content is expanded and there's margin content, does it now float off to the side? How do you think we should handle that use-case?

[AakashGfude]

Sounds good - can you add a tag or a comment for the things that we think could be upstreamed in the future, so we know where we'd like to continue removing functionality and pushing it upstream?

I have added to the top description here. We can create an issue in pydata?

The only problem with this is margin content. If the content is expanded and there's margin content, does it now float off to the side? How do you think we should handle that use-case?

I have handled it now, by removing flex-grow:1 property of pydata for bd-main class. It was useful in pydata for center aligning the content in landing page where there is neither right or left toc.

[AakashGfude]

@choldgraf some of the html theme options are deprecated in this PR, as we are directly inheriting sidebar primary from pydata sphinx theme. The deprecated theme options include:

• navbar_footer_text
• extra_navbar

I think navbar_footer_text was supposed to be deprecated after a few release cycles? And maybe extra footer as well? Let me know.

Also, single_page option has also been deprecated here, as single_page option just removes the left toc on build. For which, we already have a button to toggle left toc. The changes done by Chris also indicates the willingness to deprecate single_page option.

[AakashGfude]

Also, I think we should keep the announcement banner original color combination? Or pydata announcement banner color is good enough?

[choldgraf]

The deprecated theme options include:

• navbar_footer_text
• extra_navbar

I think it's fine for us to deprecate these for now, and wait until there's demand and/or an upstream implementation to support it.

Also, single_page option has also been deprecated here, as single_page option just removes the left toc on build.

Yes let's deprecate this as well, and inherit upstream from pydata if a default pattern gets defined there.

Also, I think we should keep the announcement banner original color combination? Or pydata announcement banner color is good enough?

Yeah let's keep the same primary color we were already using, but we can set it by over-riding the pydata theme banner color CSS variable.

Here are some broken things that I noticed after a brief scan of the documentation:

The search page margins are wrong

The left sidebar hide animation doesn't "fade" out

It used to "fade" out and in as it was moving, it now moves, then disappears at once. Can we re-introduce the fade?

Make search the docs a button

The pydata theme now has functionality for a "search bar pop-up". I suggest that we use the primary sidebar "Search the docs" field instead as a button (so it behaves like a button and when you click it, the pop-up from the pydata theme shows). That way when you're on the search page, there aren't two active fields at once, there's only the "search page search field" and the one in the primary sidebar is a button.

On this page does not fade out/in

Our On this page element used to fade out and in, and it now snaps out and in at once:

Full width code cells do not have the correct border

It cuts off part-way through the screen:

Margin content pushes code cells down

e.g.: https://sphinx-book-theme--640.org.readthedocs.build/en/640/reference/special-theme-elements.html#more-content-after-the-margin-content

Cell scrolling doesn't work

There used to be a scrollbar for long code cell outputs w/ a tag:

ref: https://sphinx-book-theme--640.org.readthedocs.build/en/640/notebooks.html#scrolling-cell-outputs

[AakashGfude]

@choldgraf thank you for uncovering these issues, it should all be fixed now, apart from the button for the search.

The pydata theme now has functionality for a "search bar pop-up". I suggest that we use the primary sidebar "Search the docs" field instead as a button (so it behaves like a button and when you click it, the pop-up from the pydata theme shows). That way when you're on the search page, there aren't two active fields at once, there's only the "search page search field" and the one in the primary sidebar is a button.

Should we remove it from the primary sidebar and place the button in the header article like in pydata-sphinx-theme?

[choldgraf]

Good question - I flip flopped on that too :-)

I suspect that if we move the search button to the header, we'll get people asking to add it back in to the sidebar. So maybe as a start we:

• Add a component template for a search icon, and make it default to the header (what you propose above)
• Add a component template to for a search bar button, and document it as an option for people who want the search bar in the sidebar

Over time, we can then explore the right UI/UX around those two

Also - a quick thought on the right TOC - when it shows up and hides, the vertical spacing also changes and causes the scroll bar to temporarily pop up. It feels a bit distracting. Could you try just making it fade in and out, without vertical changes, and see if that feels less jumpy? Or is the vertical change required in order to have the UX we want?

[AakashGfude]

I suspect that if we move the search button to the header, we'll get people asking to add it back in to the sidebar. So maybe as a start we:

* Add a `component` template for a search icon, and make it default to the header (what you propose above)

* Add a `component` template to for a search bar button, and document it as an option for people who want the search bar in the sidebar

Or how about, for now, we just hide the search bar in the search page, so that we have only the search bar on the sidebar? which mimics the current sbt. So, that we can release this PR. I can make an issue then for the proposed enhancement and we can iterate over a separate PR. Feel like the changes are already a lot in this one.

Also - a quick thought on the right TOC - when it shows up and hides, the vertical spacing also changes and causes the scroll bar to temporarily pop up. It feels a bit distracting. Could you try just making it fade in and out, without vertical changes, and see if that feels less jumpy? Or is the vertical change required in order to have the UX we want?

Hmm, the same is happening in the current sbt https://sphinx-book-theme.readthedocs.io/en/latest/content-blocks.html , I can give it a quick look if it can resolved easily here.

[mmcky]

@AakashGfude I have taken a look and compared the docs site. Just wondering about a few differences I can see.

The colours seem to be more transparent on the new version (relative to the current live site). Is this inherited from pydata-sphinx-theme? For example, the black is now grey for titles and the blue links are light blue rather than a dark blue.

[mmcky]

There is a bit more padding for code blocks

[mmcky]

I noticed a few items are being deprecated in theme options like single_page. They are no longer in the docs page, if a user specifies it (in a current project) and upgrades the theme will they be issued a deprecation notice?

[mmcky]

I like the new top bar style. I think it's a big improvement.

[choldgraf]

re: @AakashGfude

Or how about, for now, we just hide the search bar in the search page, so that we have only the search bar on the sidebar? which mimics the current sbt. So, that we can release this PR.

I think that's a great idea - let's keep this an iterative change and make a more specific functionality change in a follow-up that doesn't block this release.

The colours seem to be more transparent on the new version (relative to the current live site). Is this inherited from pydata-sphinx-theme?

Yep I suspect we are inheriting some new colors from the pydata theme (which now has light/dark mode support so colors are all defined with CSS variables). Do we need to change something?

[AakashGfude]

I noticed a few items are being deprecated in theme options like single_page. They are no longer in the docs page, if a user specifies it (in a current project) and upgrades the theme will they be issued a deprecation notice?

That's not a bad idea. have we done anything similar in pydata, and we can follow the same pattern @choldgraf ?

[choldgraf]

Definitely +1 on user-friendly deprecation notices. Where we are totally removing the functionality, we can check for the configuration value and add a message like "This was removed in version XXX, for similar functionality see these docs."

[mmcky]

Yep I suspect we are inheriting some new colors from the pydata theme (which now has light/dark mode support so colors are all defined with CSS variables). Do we need to change something?

No I had a slight preference for the bolder colours but it is not a big issue :-).

[choldgraf]

@mmcky Note that I think the pydata theme may actually be changing its default colors soon to be more a11y friendly, so I think that might make the colors a bit bolder:

• https://github.com/pydata/pydata-sphinx-theme/issues/1052

[AakashGfude]

I did a deprecation notice warning like these:

But sphinx also gives warnings for theme options that are not present in theme.conf, like so:

Seems like duplication.

[choldgraf]

Three quick things:

• I think it's OK to duplicate the warning, because it's useful to know that it has been deprecated. The Sphinx warning just tells you there's an error, but not why.
• I'd recommend providing a URL to a specific page where people can learn more, rather than just a generic link to the docs.
• If we're not sure where that page should be, then I'd recommend linking to a section of the CHANGELOG with migration instructions, or a little wiki page that describes how to migrate workflows

[AakashGfude]

* I'd recommend providing a URL to a specific page where people can learn more, rather than just a generic link to the docs.

* If we're not sure where that page should be, then I'd recommend linking to a section of the CHANGELOG with migration instructions, or a little wiki page that describes how to migrate workflows

• Okay so for keys 'navbar_footer_text', and 'extra_navbar'. 'navbar_footer_text' was just an old name for extra_footer and was kept for backward compatibility from what I can see. extra_footer adds an extra HTML at the bottom of the right sidebar. the default value of which is : Theme by the <a href="https://ebp.jupyterbook.org">Executable Book Project</a>

The default value seems like a duplication of the footer-content on the right side of it. And also pydata has a similar option if needed: https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/layout.html#primary-sidebar-end-sections. I guess we can link this doc for that warning?

• the alternative for single-page is this button, which has no doc link obviously:-

So, for extra_navbar I reckon we link the pydata doc. And for single-page, maybe a CHANGELOG? navbar_footer_text was anyways meant to be deprecated in a few release cycles from the comments.

[choldgraf]

Quick thoughts:

Okay so for keys 'navbar_footer_text', and 'extra_navbar'. 'navbar_footer_text' was just an old name

I'm OK just removing this, and note that users can do this on their own by creating their own sidebars rules in conf.py

And for single-page, maybe a CHANGELOG

Sounds good

[choldgraf]

A few more things that I noticed (listed below). I think that we are getting close! We should focus on cleaning up any remaining visual bugs and then we can iterate on other functionality in subsequent PRs.

@mmcky could you also have a thorough look-through to make sure it looks OK?

The primary sidebar is a little too wide

Our primary sidebar is now slightly too wide (I think...do others agree?) - here's the difference:

There's a weird layout in certain middle-sized widths:

I think our responsive rules aren't quite right, see this for example:

Hanging left padding in margin content

The margin content title underline hangs to the left, I think instead we should expand the content to be flush with the title content

And an example with a code block

Note inside sidebar and margin has strange formatting

and in margin:

both the borders seem to be off, and the padding on the left/right is off

Could we use the pydata scrollbar style for the right TOC?

And while you're at it, I think it should fade in/out with opacity as well as the max-height

Here's the mixin where the scrollbar is defined:

https://github.com/pydata/pydata-sphinx-theme/blob/main/src/pydata_sphinx_theme/assets/styles/abstracts/_mixins.scss#L9-L26

"previous" and "next" should be flush with the page content

Instead they spill out to the left and right. E.g.:

Literal span elements spill out to the right

This one's a bug upstream as well so let's just fix it there, I opened this issue to track:

• https://github.com/pydata/pydata-sphinx-theme/issues/1076

[AakashGfude]

The primary sidebar is a little too wide

Our primary sidebar is now slightly too wide (I think...do others agree?) - here's the difference:

That is true. The width is almost similar to pydata primary sidebar. Do we want to make the same as the original sbt? We will also have to decrease font-size etc.

Hanging left padding in margin content

The margin content title underline hangs to the left, I think instead we should expand the content to be flush with the title content

I have made it behave same as original sbt now, styling that way was easier:-

Note inside sidebar and margin has strange formatting

corrected now

Note that the original sbt one was wider occupying the secondary sidebar. But this one is following the pydata sidebar style. Should that be okay?

and in margin:

should be okay now:

Could we use the pydata scrollbar style for the right TOC?

And while you're at it, I think it should fade in/out with opacity as well as the max-height

It's fading with max-height at the moment. The fade in/out should be the same as original sbt. Do we want to add an extra effect of opacity?

Also, I thought it was already inheriting the pydata style:: Left: pydata, Right: this PR

"previous" and "next" should be flush with the page content

should be okay now?

[AakashGfude]

Quick thoughts:

Okay so for keys 'navbar_footer_text', and 'extra_navbar'. 'navbar_footer_text' was just an old name

I'm OK just removing this, and note that users can do this on their own by creating their own sidebars rules in conf.py

And for single-page, maybe a CHANGELOG

Sounds good

So, have just kept the single_page warning then? Something like this:

[choldgraf]

Thanks @AakashGfude - those fixes look great. Here are some responses / things I noticed as well:

Primary sidebar width

The width is almost similar to pydata primary sidebar. Do we want to make the same as the original sbt? We will also have to decrease font-size etc.

Let's shrink the sidebar to at least get close to the old behavior of the book theme. We don't necessarily have to shrink the font size by much, only if it feels cramped. Let's take a look and see what it looks like? You can also use your best judgment there.

Secondary sidebar scroll style

Here's what it looks like on my machine - it doesn't seem to be inheriting the scroll style of the pydata theme. I think this might be related to the double scrollbar issue I mention below. The secondary sidebar itself has the right style, but the "on this page" list of items also has a scrollbar that is not styled

I think to fix this we could just:

• Get that scrollbar style working
• Shorten the animation time a bit so that it's faster (it feels slower than the same animation in the old SBT)

Code block in margins

Could you remove the white background that is in the scrollbar for code blocks in the margins?

Here's what it looks like in the pydata theme:

Double sidebars on mobile

I noticed that we have some kind of "double sidebars" effect going on in the mobile drawer for the secondary sidebar. There's a sidebar for the list of pages, the sidebar itself, and the page all by each other.

Deprecation warning

So, have just kept the single_page warning then? Something like this:

I'd add the actual URL for the changelog. We want to minimize the number of actions that a user must take to go from that error message to a page w/ the solution.

Audit CI/CD build

Can you relax our testing upper-bound a little bit, so that it doesn't fail? I think we can do follow-up work to improve accessibility but don't need to block this now.

[AakashGfude]

@choldgraf , not sure why my system does not show the double sidebar. Have added back the mixin and added them. Do they look okay or there are two stylized sidebars now?

Have adjusted the primary sidebar width and CI/CD warning.

I'd add the actual URL for the changelog. We want to minimize the number of actions that a user must take to go from that error message to a page w/ the solution.

Shouldn't I be adding an entry in the CHANGELOG about this PR, in the release PR? Or should I just add it here?

[AakashGfude]

@choldgraf Are you using safari? We still have that browser stack do we?

[choldgraf]

Hmmm - let me give it a shot on my machine and see if I can clean up those rough edges. I'll push a commit to this PR when I'm done and let you know what I changed. Might take a bit of fiddling as you have learned :-)

[AakashGfude]

@choldgraf thank you! that would be really helpful. I have also pushed the responsive CSS. let me know how it looks.

[choldgraf]

Sounds good - I'll pull your changes in and will try to take another pass at fixing this up + removing some other cruft as much as possible.

[choldgraf]

OK I think that I got the behavior working properly. I also took a stab at our button CSS and removed the custom button code we had, and instead am re-using Bootstrap's button HTML structure so that we aren't defining things in a custom way. I had to fiddle a bit with the margin behavior but I think I got that working as well.

To be honest, I suspect that there are more things that will need tweaking and fixing, but IMO we should get this one in soon because we've already worked on it a lot.

@AakashGfude could you take a final pass through the docs and see if there's anything obviously wrong with them. If it's a quick fix we can just go for it, but if not then we can just merge and iterate in future PRs.

Also discovered some UI bugs in the pydata theme, but these are related to Bootstrap 5 and so we can merge this before them:

• https://github.com/pydata/pydata-sphinx-theme/issues/1082

[mmcky]

thanks @choldgraf.

@AakashGfude let me know when you will get to review and merge this PR. Thanks. Looking forward to looping around the other projects once this is released.

[choldgraf]

Two quick things that I noticed:

Button alignment is off w/ the toggles

Dropdown menu click events don't seem to be working

If I click the "launch" or "download source" buttons it doesn't show - there's probably an HTML or CSS bug in there. Alternatively we could also re-introduce the behavior of "show on hover" there - @AakashGfude what do you think?

Once we get those done, I suggest we merge this and then make a pre-release of this theme so that @mmcky can try it out

[AakashGfude]

Thanks for the changes @choldgraf , I have taken a stab on fixing those two issues. Let me know how it looks. I will look through the preview.

Also, I have noticed that in width greater then around 990px and less then 1200px, the secondary sidebar is no longer visible:

Is that intentional? Or should we have the behaviour like present sbt.

[AakashGfude]

@choldgraf The site looks okay to me. Apart from the thing mentioned in the previous comment.

[choldgraf]

I tweaked two items that I noticed errors on:

• On mobile, clicking the menu buttons would cause the Bootstrap tooltip to show over the drawer. I added a rule to ensure it is just below the two drawers.
• Margin notes had no left margin on mobile which caused them to touch the content, I added the same margin we use for .sidebar content.

I am sure that there is more to improve, but I suggest we merge this, and any other outstanding PRs that are ready to go, and then cut a pre-release so that @mmcky can try the upgrade in jupyter book more easyil

[choldgraf]

Thanks for helping to push this through @AakashGfude !