-
Notifications
You must be signed in to change notification settings - Fork 326
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
✨ Using sphinx toc functions #219
Conversation
647eff3
to
b381823
Compare
6a10768
to
7027c90
Compare
src/scss/index.scss
Outdated
@@ -283,35 +288,26 @@ table.field-list { | |||
} | |||
} | |||
|
|||
// For page headers with toctrees under them | |||
li.page-header { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just simplifies the CSS a little bit - we were using a bunch of level-specific selectors for our rules, but I think we can get the same effect with the generic rules here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you remember where/when this "page-header" class is used? I don't directly find it in the output HTML somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@choldgraf I don't see this "page-header" class use anywhere, so for now going to remove it again. If it actually had a purpose, please explain and I will add it back :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go for it - I have no idea what I was thinking here because this PR is from 7 months ago :-)
</ul> | ||
|
||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we weren't closing out this div
before 🤷
7027c90
to
7b56b8b
Compare
Friendly ping on this - this PR is also fixing a pretty big bug that we're experiencing in the Jupyter docs (pages that are > 2 levels deep don't show up in the sidebar). I could try to implement the fix with our current docutils approach but it seemed like this PR would actually be a simpler way to solve the bug and solve ourselves some headache in the future as well... |
(I am only just back, will try to take a look at this tomorrow, but from the description I am not fully fond of the idea ... but maybe we should have a call about it one of the coming days to easier discuss it?) |
I'm happy to take a call to chat it through. This PR was mostly borne out of me being frustrated with debugging issues over and over again in our custom docutils parsing code :-) My personal opinion is that this approach is much, much simpler than what we're currently doing. Rather than replicating 80% of Sphinx's Basically, here's what we do now:
and this PR makes it:
(FWIW, I'm also happy to chat on a quick call now if you wanna chat it through) |
@jorisvandenbossche I just pushed an update to this PR that adds the ability for the I also tested out how this could be used by a theme by trying it out in the In particular, see how I'm calling I'm then just doing some minor editing to that BeautifulSoup object and then returning my own HTML. What do you think? I can develop in this direction a bit further if you think it's a nice compromise. |
Just a note that I'm just gonna implement this for the SBT theme. I'd really prefer to upstream this but I can't wait super long to get it in. Here's the PR that we can use for reference in the future: executablebooks/sphinx-book-theme#130 Note that as I was implementing that PR, I also realized that sphinx-style page / section numbering also doesn't work in this theme (unless we manually add it in), so we should document that (or get this PR merged) |
OK, no problem, that way it can be test drived in jupyter-book-theme before upstreaming it ;-) Sorry for the delay, and leaving now for holidays. But as I said on our call: I am certainly on board with the idea! (but I haven't yet been able to test it with some other projects) |
@jorisvandenbossche what are your thoughts on this? FWIW we've had this in jupyter book for a few months now and it has worked fine. |
FWIW, this PR changed numpy document build time (without multiprocessing via |
I wouldn't expect much performance change unless beautifulsoup is being extremely inefficient somehow...that's weird that said, I'm on paternity leave now and don't have the time to keep rebasing this PR as the pydata theme changes, and it doesn't seem like folks are super interested in this functionality, so I am not planning to put a lot more work into it (though will note again that this has simplified things a lot in the sphinx-book-theme). |
pydata_sphinx_theme/__init__.py
Outdated
for i in range(1, context["theme_show_toc_level"] + 1): | ||
for li in soup.select("li.toc-h{}".format(i + 1)): | ||
ul = li.find_parent("ul") | ||
classes = ul.get("class", []) | ||
if "visible" not in classes: | ||
ul["class"] = classes + ["visible"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@choldgraf this is "a" way that I found to get show_toc_level
working, but I find it kind of hacky ..
The problem with the basic
for ul in soup("ul"):
ul["class"] = ...
loop above this loop is that this is a flat iterator, and so you don't know how "deep" you are for each ul
.
This "workaround" uses the toc-hN
class names we added above, to find the appropriate levels of ul
for which to add the class. I didn't directly find a way with beautifulsoup to only search a certain number of levels down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems reasonable to me :-), but maybe we should add a comment to explain your reasoning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure why I missed that earlier. But of course the code that adds those toc-hN
classes (the classes that I was then using here to navigate the soup as a workaround), is already recursively iterating through the lists keeping track of the level ... :)
So simplified this
thanks for the pushes! can you update your "to do" list to reflect the things that you've tackled here? |
@jorisvandenbossche do you think that we can leave:
to another PR? It feels to me like a feature request whereas this PR is meant to be a more "under the hood" change that doesn't affect end functionality but makes the theme less buggy. Then we can just merge this one now so it's easier to iterate with smaller updates. What do you think? |
ping @jorisvandenbossche - I wanna get this PR merged before it goes stale again but I don't have the bandwidth to implement the extra logic for the sidebar right now... |
@choldgraf reviving this. This PR certainly doesn't need to do everything that is possible with the new functions.
|
This is actually something you fixed in executablebooks/sphinx-book-theme#215, so copied that part of the PR here. |
So it sounds like we are good to merge this, and then we'll need to re-implement #135 using the new setup, right? |
OK, this is merged ;) I further removed the header-page CSS, renamed the new functions that returns the HTML to make the name more correct (
Indeed, next steps are adding support for captions, and I also want to add support for collapsing subsections in the sidebar. |
hooray! what a journey :-) are there issues for collapsible sidebar items etc? We should make sure not to lose those threads (and many may be upstreamable from the book theme) |
Support for captions and collapsing subsections~ Wohh sounds wonderful. 👍 |
This PR refactors our navigation bar, sidebar, and table of contents logic to use Sphinx toc functions instead of our own custom parsers. However, the output shouldn't change because it uses
beautifulsoup
to parse the HTML that Sphinx gives us in order to add the right classes and structure.In implementing this, I also noticed some other bugs that our old parsing code had, that we didn't notice. (e.g., in our demo page, clicking the api documentation sidebar link doesn't expand that page's children). This should fix those bugs too since we're just using the basic Sphinx sidebar functionality, nothing custom.
Pros
:numbered:
)Cons
beautifulsoup
(which I hope isn't too bad, since that is such a standard in the Python world)@jorisvandenbossche would love to know what you think here
resolves #220 #110
also can resolve #218 #135 (but needs tests)