-
Notifications
You must be signed in to change notification settings - Fork 5
Re-add toc navigation #682
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
Conversation
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.
Thank you for submitting a patch without the Sass updates, so we can focus on the subject matter itself.
NB: This patch lacks a changelog item in CHANGES.rst, what the previous one had. In there, don't hesitate to be more explicit about mentioning the migration of the primary navigation representation from HTML/Jinja to Python, e.g. like a compressed version of "Use a / Switched to a built-in Sphinx extension to define the primary navigation instead of an HTML/Jinja template construct", instead of dismissing this significant change as just "advanced navigation". Thanks!
src/crate/theme/rtd/__init__.py
Outdated
| def setup(app): | ||
| """ | ||
| Registers event handlers to setup navigation. | ||
| """ | ||
| app.connect("html-page-context", _add_crate_navigation) | ||
| return { | ||
| "version": __version__, | ||
| "parallel_read_safe": True, | ||
| "parallel_write_safe": True, | ||
| } |
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.
The theme has a def setup entrypoint already. Is your intention to introduce another one, to be able to slot in those features on behalf of a dedicated Sphinx extension?
In this case, please don't twist hierarchies, and move this def setup downwards towards the extension itself, not above the main def setup entrypoint of the whole theme, located in conf/__init__.py.
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.
Long story short, please move all initializer code into sidebartoc.py itself, so it can become a self-contained Sphinx extension like possibly intended.
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.
The code was written "last year"... Not sure why I had the "extension" part there still. I've cleanup the setup parts a bit now.
amotl
left a comment
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.
Thanks for the streamlining. I am acknowledging now.
Next time, please don't forget to re-request a review,
otherwise there is the danger about waiting for ages, because I don't get any notification about it, and the item is also not included on the canonical page where to review requested reviews [sic].
…ate-docs-theme into new-toc-navigation-pure
|
CHANGES.rst
Outdated
| - (re)Added TOC expand/collapse icons and more advanced navigation. The expanded | ||
| or collapsed state of individual TOC entries is now persisted and you can | ||
| therefore keep multiple sections expanded at the same time while navigating | ||
| pages. A sideeffect is that the TOC state is now also persisted across page | ||
| reloads. | ||
| Not mentioned previously, it also introduce a significant change to how the | ||
| sidebartoc is defined. Previously it was defined in HTML/Jinja templates, | ||
| now it is generated in Python code. | ||
| This was first introduced in 0.43.0, but reverted in 0.44.0. |
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.
Can you phrase this less of a story, i.e. more concise? /cc @coderabbitai
CHANGES.rst
Outdated
| - (re)Added TOC expand/collapse icons and more advanced navigation. The expanded | ||
| or collapsed state of individual TOC entries is now persisted and you can | ||
| therefore keep multiple sections expanded at the same time while navigating | ||
| pages. A sideeffect is that the TOC state is now also persisted across page | ||
| reloads. |
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.
"Persisted" is used twice. This whole paragraph can be phrased more concise.
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.
yeah - what a wordy mess :-).
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.
Pull request overview
This PR re-adds TOC expand/collapse navigation icons that were previously reverted in version 0.44.0. The implementation migrates from Jinja2 template-based navigation generation to a Python-based approach using a new sidebartoc.py module, which enables integration with Furo's navigation enhancer for collapsible icons and checkboxes. The navigation state is now persisted across page loads using localStorage.
Key changes:
- New Python module
sidebartoc.pygenerates navigation HTML and processes it through Furo's navigation enhancer - JavaScript added for navigation state persistence and expand/collapse behavior
- CSS/SCSS styling for Furo-compatible collapsible navigation
- Template simplification with navigation generation moved from Jinja to Python
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/crate/theme/rtd/sidebartoc.py | New Python module that generates multi-project navigation HTML and enhances it with Furo's navigation tree for collapsible icons |
| src/crate/theme/rtd/crate/static/js/custom.js | Added navigation state persistence logic using localStorage and click handlers for expand/collapse behavior |
| src/crate/theme/rtd/crate/static/css/furo-collapsible-toc.scss | New SCSS file with styling for Furo-compatible collapsible navigation elements |
| src/crate/theme/rtd/crate/static/css/index.css | Adds import for new furo-collapsible-toc.scss file |
| src/crate/theme/rtd/crate/static/css/crateio-rtd.css | Removes old CSS rules for TOC display that are now handled by Furo's navigation enhancer |
| src/crate/theme/rtd/crate/sidebartoc.html | Simplified to just render the enhanced navigation from context variable |
| src/crate/theme/rtd/crate/sidebar.html | Added 'sidebar-tree' class to nav element for CSS targeting |
| src/crate/theme/rtd/conf/init.py | Registers the new Sphinx event handler for navigation enhancement and adds comment about SCSS bundling |
| src/crate/theme/rtd/init.py | Refactored utility functions to eliminate redundant current_dir variable shadowing |
| docs/tests/*.rst | New test documentation pages for verifying navigation behavior |
| docs/index.rst | Reordered toctree entries and added tests/index |
| CHANGES.rst | Documents the re-addition of TOC expand/collapse icons |
| .gitignore | Adds pattern to ignore underscore-prefixed files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Do you know why Copilot is now active here, but @coderabbitai does not respond any longer? |
|
I don't know why coderabbit isn't responding. But I manually summoned Copilot by requesting it as reviewer in the absense of the rabbit. |
Summary of the changes / Why this is an improvement
Adding TOC expand-collapse icons again (as #664 was reverted).
This is a variant of #681 that just includes the navigation icons and avoids removing the sass build warnings.
Preview
https://crate-docs-theme--682.org.readthedocs.build/en/682/