Skip to content

Fix: Correctly highlight active navigation link on scroll - #44

Open
black-hat-star00 wants to merge 1 commit into
ladunjexa:mainfrom
black-hat-star00:fix-navbar-highlighting
Open

Fix: Correctly highlight active navigation link on scroll#44
black-hat-star00 wants to merge 1 commit into
ladunjexa:mainfrom
black-hat-star00:fix-navbar-highlighting

Conversation

@black-hat-star00

Copy link
Copy Markdown

The previous implementation had a logic flaw where the active navigation link would not be cleared correctly when scrolling between sections. This was because the setActive function was only called when a section was in view, but it was not cleared when no section was active.

This commit refactors the scroll event handler in Navbar.tsx to correctly identify the active section and clear the highlighting if no section is active, regardless of the scroll position. This ensures that the active link is always correctly highlighted.

The previous implementation had a logic flaw where the active navigation link would not be cleared correctly when scrolling between sections. This was because the `setActive` function was only called when a section was in view, but it was not cleared when no section was active.

This commit refactors the scroll event handler in `Navbar.tsx` to correctly identify the active section and clear the highlighting if no section is active, regardless of the scroll position. This ensures that the active link is always correctly highlighted.
@changeset-bot

changeset-bot Bot commented Oct 1, 2025

Copy link
Copy Markdown

鈿狅笍 No Changeset found

Latest commit: e3f01b7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel

vercel Bot commented Oct 1, 2025

Copy link
Copy Markdown

@google-labs-jules[bot] is attempting to deploy a commit to the ladunjexa's team Team on Vercel.

A member of the Team first needs to authorize it.

@black-hat-star00

Copy link
Copy Markdown
Author

hii馃榿

@ladunjexa ladunjexa left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @black-hat-star00 ,
just reviewed your commit, and it looks good. but there is a much cleaner and efficient way to handle navbar highlighting logic than using the getBoundingClientRect, and this magic done by the IntersectionObserver API.
It's all about using a single useEffect (mount effect, empty deps) to create the new observer, iterate over the observed entries and check entry.isIntersecting to determine which section is currently visible, and maintain current using a single state to store the active session, instead of using * 0.2, you can use the threshold which represents the % of visibility to trigger the callback - and that's it; simply select elements (queryselector) and call observe on each of them, don't forget to cleanup by unobserving them.

Anyway If you prefer not to make this change, it's totally fine since it's an improvement over the existence - just remove the unnecessary files (the test folder you've committed), and I'll merge it as is. Thanks for your contribution 馃憤

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