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

MAINT: Remove duplication with pydata-sphinx-theme #640

Merged
merged 78 commits into from
Jan 5, 2023

Conversation

choldgraf
Copy link
Member

@choldgraf choldgraf commented Nov 7, 2022

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

  • Make changes to support Bootstrap 5 (ref: MAINT: Drop jQuery and use Bootstrap 5 pydata/pydata-sphinx-theme#1029)
  • See if there's anything else we couple possibly remove from this theme that is supported in the PST
    • Try removing all of our button / menu CSS and re-using the PyData / Bootstrap rules
  • Fix up our CSS in order to make the end-user UI behavior behave the same way as before
    • The right table of contents button in the article header should only show on mobile screens
    • Ensure that behavior showing/hiding the table of contents w/ margin content present is correct
  • 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
  • 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

@AakashGfude
Copy link
Member

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
Copy link
Member Author

choldgraf commented Nov 10, 2022

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
Copy link
Member Author

choldgraf commented Nov 22, 2022

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.

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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?

Screen Shot 2022-11-23 at 12 03 26 pm

@choldgraf
Copy link
Member Author

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:

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?

@choldgraf
Copy link
Member Author

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
Copy link
Member Author

choldgraf commented Dec 14, 2022

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:

@mmcky
Copy link
Member

mmcky commented Dec 17, 2022

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
Copy link
Member Author

Two quick things that I noticed:

Button alignment is off w/ the toggles

image

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
Copy link
Member

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:

Screen Shot 2022-12-20 at 7 37 57 pm

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

@AakashGfude
Copy link
Member

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

@choldgraf
Copy link
Member Author

choldgraf commented Jan 5, 2023

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 choldgraf changed the title MAINT: Removing duplication with pydata-sphinx-theme MAINT: Remove duplication with pydata-sphinx-theme Jan 5, 2023
@choldgraf choldgraf merged commit e99dea1 into executablebooks:master Jan 5, 2023
@choldgraf
Copy link
Member Author

Thanks for helping to push this through @AakashGfude !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants