Skip to content
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

Fix #29 remove active class on first item in nav, when scrolled back up #56

Merged
merged 1 commit into from
Jul 6, 2024

Conversation

sulliops
Copy link
Contributor

This is a small patch that fixes an issue identified in #29 where the first menu item's active class isn't removed when scrolling back up to content on the page before the section whose ID corresponds to that menu item.

Demo of the incorrect behavior (as previously requested in #29):

scrollspy-bug-demo.mp4

In this demo, I removed the active class on the "Hero" menu link and changed the ID of the Hero section to something other than "hero". When scrolling past the Hero section in this demo, the active class is correctly added to the "Section 1" menu item; when scrolling back up to the Hero section, the active class is not removed.

I determined that there exists a condition where the plugin finds a 0 value for a section's starting position but still returns that section as being in view because the document's scroll position is greater than 0.

Additionally, I'm not familiar with TypeScript so it may be worth reviewing what few changes I made for type safety. In particular, one of my modifications changes type information to accept a null value. There may be a better way to accomplish my goal that I'm not aware of, I just needed a quick and dirty solution to be able to use this plugin properly.

Cheers for the great work, found this plugin after struggling with Bootstrap's built-in (and broken) Scrollspy plugin!

Modified behavior of getSectionInView(), onScroll(), and removeCurrentActive() to remove active class on first menu item when that item corresponds to a section below other content
@kimyvgy kimyvgy changed the base branch from master to fix/active-status July 6, 2024 04:09
@kimyvgy
Copy link
Owner

kimyvgy commented Jul 6, 2024

@sulliops It seems the demo is working incorrectly. Let's me add some changes for this fix before merging into master branch. Thanks for your work 👍

@kimyvgy kimyvgy merged commit e304263 into kimyvgy:fix/active-status Jul 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants