-
-
Notifications
You must be signed in to change notification settings - Fork 25
👌 IMPROVE: RHS TOC sticky when scrolling option #144
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
|
nice work @DrDrij -- it would be good to get the preview building with the updated RHS toc. Is that possible? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
=======================================
Coverage 71.12% 71.12%
=======================================
Files 2 2
Lines 239 239
=======================================
Hits 170 170
Misses 69 69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@DrDrij is the outstanding checkbox still in work? |
|
re: The main toc is So perhaps we need to differentiate these |
|
@DrDrij what do you think re: comments / names? |
|
@DrDrij do you think you would have some time in the next two weeks to see if we can close this? |
|
@mmcky Great idea. Changed the parameter to I think that covers it for this PR :) |
|
@AakashGfude the latest |
|
@AakashGfude let's revert 0acf72c and I will update the |
|
@AakashGfude I just pushed c8fd78a |
|
@AakashGfude once you get the tests and merge conflict sorted -- then please ping me for review. Thanks. |
|
@AakashGfude this looks sticky now 👍 What do you think? I suspect @jstac will prefer the non-sticky version but it is nice that our theme can support this option. |
|
@jstac this PR enables the RHS within page contents to stay visible in the margin https://64546294cce6883938fb185c--hungry-lovelace-7d0512.netlify.app/python_by_example.html Let me know your thoughts on this? I am in two minds - I like the clean view of it just being at the top (i.e. not sticky) but it can be handy to navigated within the page with it available all the time. |
|
Thanks @mmcky. This looks good now. Yes, I think by default we should keep it non-sticky. Good to have the option. |
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 implements a sticky right-hand side (RHS) table of contents (TOC) feature with scroll-based highlighting for the QuantEcon Book Theme. The feature is configurable through a new theme option and defaults to disabled to maintain backward compatibility.
Key changes include:
- Added a new configuration option
sticky_contentsthat defaults toFalse - Implemented CSS positioning to make the TOC sticky when the option is enabled
- Added JavaScript scroll spy functionality to highlight current section in TOC
- Removed an existing CSS rule that was interfering with the new navigation behavior
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scss/quantecon-book-theme.scss | Added CSS for sticky positioning and active state styling for TOC items |
| src/js/quantecon-book-theme.js | Added ScrollSpy plugin and initialization for TOC highlighting |
| src/jinja/theme.conf.j2 | Added sticky_contents configuration option with default False |
| src/jinja/theme.conf | Added sticky_contents configuration option |
| quantecon_book_theme/theme.conf | Updated compiled theme configuration and stylesheet reference |
| quantecon_book_theme/static/*.css | Compiled CSS assets with new sticky TOC styles |
| quantecon_book_theme/static/*.js | Compiled JavaScript with ScrollSpy functionality |
| quantecon_book_theme/layout.html | Added conditional sticky class based on theme configuration |
| .pre-commit-config.yaml | Added MarkupSafe dependency pin for compatibility |
| .github/workflows/ci.yml | Updated Python version from 3.9 to 3.10 |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
|
||
| &.active { | ||
| >a { | ||
| //color: $color-primary; |
Copilot
AI
Aug 16, 2025
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.
Remove the commented out color property. Commented code should be cleaned up rather than left in the codebase.
| //color: $color-primary; |
|
|
||
| .nav>.active>ul { | ||
| display: block; | ||
| // display: block; |
Copilot
AI
Aug 16, 2025
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.
Remove the commented out display property. If this rule needs to be disabled, consider removing it entirely or documenting why it's commented out.
| // display: block; |
|
|
||
| .toctree-checkbox[type="checkbox"] { | ||
| position: absolute; | ||
| left: 400rem; |
Copilot
AI
Aug 16, 2025
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 value 400rem is a magic number that appears to be used to hide elements off-screen. Consider using a more semantic approach like left: -9999px or display: none with proper accessibility considerations.
| left: 400rem; | |
| left: -9999px; |
| @@ -1,3 +1,6 @@ | |||
| // ScrollSpy Plugin: https://github.com/r3plica/Scrollspy | |||
| !function($,window,document,undefined){$.fn.extend({scrollspy:function(options){var defaults={namespace:"scrollspy",activeClass:"active",animate:!1,duration:1e3,offset:0,container:window,replaceState:!1};options=$.extend({},defaults,options);var add=function(ex1,ex2){return parseInt(ex1,10)+parseInt(ex2,10)},findElements=function(links){for(var elements=[],i=0;i<links.length;i++){var link=links[i],hash=$(link).attr("href"),element=$(hash);if(element.length>0){var top=Math.floor(element.offset().top),bottom=top+Math.floor(element.outerHeight());elements.push({element:element,hash:hash,top:top,bottom:bottom})}}return elements},findLink=function(links,hash){for(var i=0;i<links.length;i++){var link=$(links[i]);if(link.attr("href")===hash)return link}},resetClasses=function(links){for(var i=0;i<links.length;i++)$(links[i]).parent().removeClass(options.activeClass)},scrollArea="";return this.each(function(){for(var element=this,container=$(options.container),links=$(element).find("a"),i=0;i<links.length;i++){var link=links[i];$(link).on("click",function(e){var target=$(this).attr("href"),$target=$(target);if($target.length>0){var top=add($target.offset().top,options.offset);options.animate?$("html, body").animate({scrollTop:top},options.duration):window.scrollTo(0,top),e.preventDefault()}})}resetClasses(links);var elements=findElements(links),trackChanged=function(){for(var link,position={top:add($(this).scrollTop(),Math.abs(options.offset)),left:$(this).scrollLeft()},i=0;i<elements.length;i++){var current=elements[i];if(position.top>=current.top&&position.top<current.bottom){var hash=current.hash;if(link=findLink(links,hash)){options.onChange&&scrollArea!==hash&&(options.onChange(current.element,$(element),position),scrollArea=hash),options.replaceState&&history.replaceState({},"","/"+hash),resetClasses(links),link.parent().addClass(options.activeClass);break}}}!link&&"exit"!==scrollArea&&options.onExit&&(options.onExit($(element),position),resetClasses(links),scrollArea="exit",options.replaceState&&history.replaceState({},"","/"))};container.bind("scroll."+options.namespace,function(){trackChanged()}),$(document).ready(function(e){trackChanged()})})}})}(jQuery,window,document); | |||
Copilot
AI
Aug 16, 2025
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 ScrollSpy plugin is minified/obfuscated code that makes debugging and maintenance difficult. Consider using a readable version during development or include a reference to the original source for better maintainability.
| }); | ||
| // Highlight page TOC links as user scrolls | ||
| $(".sticky #bd-toc-nav ul").scrollspy(); | ||
|
|
Copilot
AI
Aug 16, 2025
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 selector .sticky #bd-toc-nav ul assumes a specific DOM structure. Consider making this more robust by checking if the element exists before initializing scrollspy to avoid potential errors.
| var $tocNav = $(".sticky #bd-toc-nav ul"); | |
| if ($tocNav.length > 0) { | |
| $tocNav.scrollspy(); | |
| } |
|
@DrDrij would you mind reviewing this PR. Not sure why we didn't merge this when it was ready. I still think it would be a nice feature to have. |
|
@copilot are you able to review this PR and resolve the merge conflicts? |
|
@DrDrij clearly a lot has changed. It might be easier to open a new PR with the final changes? |
|
@copilot are you able to review this PR and update it to the most recent |
|
Closing in favour of #316 |
Fixes #133
The page TOC will now highlight as the user scrolls.
A config option is required to activate the sticky feature and highlighting functionality. This defaults to
Falsewhere the page TOC is not sticky.Screen.Recording.2021-06-22.at.4.04.08.pm.mov