-
Notifications
You must be signed in to change notification settings - Fork 5
Re-add toc navigation #681
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
base: main
Are you sure you want to change the base?
Conversation
aeea462 to
9fe0d52
Compare
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. If it's possible to remove the SASS updates, the patch will be much more concise, and will not introduce any side effects.
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.
Is it a good idea to change anything on vendorized resources? Instead, I think it's better to keep them unmodified, but regularly update them from the original sources. This remark also applies to all other resources under the vendor/furo directory.
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.
Following this advice, I think it's better to remove 9fe0d52 from this patch again. Maybe you want to submit this to Furo upstream beforehand?
Not following this advice will make it nearly impossible to update corresponding resources on future releases of Furo through updates like GH-677, which can easily introduce changes that require updating corresponding assets. Unfortunately, we can't pull them from the original project dynamically, so they have been included 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.
You are absolutely right that that is the "right" way to do it.
The issue was that the original patch for the icons depended on the changes done already for removing the deprecation warnings. So I gave it a try to get rid of all those warnings anyway. The updates to do that are relatively simple to do with the automatic upgrade tool provided: sass-migrator. So even if changes are done to Furo, we can easily apply the same upgrades I did by running that tool on top.
But I get the principle point and generally agree. I'll give it a go to see if I get easily get the icon patch updated to work on top of current main instead.
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.
Alright - that was easy: #682 includes just the nav icons now.
Summary of the changes / Why this is an improvement
Adding TOC expand-collapse icons again (as #664 was reverted).
Includes removal of build warnings that was causing the issues of past (reverted) builds. But now working correctly.
This has been tested with both the docs in the theme and the guide.
Preview
https://crate-docs-theme--681.org.readthedocs.build/en/681/