-
Notifications
You must be signed in to change notification settings - Fork 200
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
MAINT: Remove duplication with pydata-sphinx-theme #640
Conversation
for more information, see https://pre-commit.ci
…inx-book-theme into maint-remove-cruft
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):
|
Just a quick note that:
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 |
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. |
for more information, see https://pre-commit.ci
…inx-book-theme into maint-remove-cruft
@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. |
Few things I had to add back because they are not yet in pydata:
|
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:
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 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? |
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. |
for more information, see https://pre-commit.ci
…inx-book-theme into maint-remove-cruft
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: |
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. |
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 |
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. |
@choldgraf The site looks okay to me. Apart from the thing mentioned in the previous comment. |
I tweaked two items that I noticed errors on:
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 |
Thanks for helping to push this through @AakashGfude ! |
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
git+
section of our dependencies so we aren't installing frommain
Few things I had to add back because they are not yet in pydata:
References