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

ScrollSpy: improve active feedback #41016

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

guilhas07
Copy link

@guilhas07 guilhas07 commented Nov 7, 2024

Motivation & Context

Current ScrollSpy behavior had some shortcomings when it comes to detecting the correct active class.

  • Sometimes none of the classes are active
  • After clicking to go to a section, it immediately shows as non active

Solution

To fix this I simplified the current logic, by adding an active state for all sections and keeping track of the previous highlighted section. The current rational is to highlight the first section that is intersecting at all times.

Different logic for scrolling up or down and custom thresholds are removed due to not being needed anymore.

Note: This is a first draft, so I didn't delve deeper into all of the code that can be simplified. I would be grateful for any feedback. Thank you for your time and for this amazing project!

Changes

Scroll Behaviour:

  • Before ( Note that sometimes the active class disappears and other times it jumps to early ):
old-scroll.mp4
  • After:
new-scroll.mp4

Click Behaviour:

  • Before (Note the section clicked isn't active after auto scroll)
old-click.mp4
  • After
new-click.mp4

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Refactoring (non-breaking change)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live Previews

https://deploy-preview-41016--twbs-bootstrap.netlify.app/docs/5.3/getting-started/introduction/

@guilhas07 guilhas07 requested a review from a team as a code owner November 7, 2024 17:22
@Manchmal1
Copy link

Was muß ich machen um die auf dem Fernseher zu sehen bitte

@julien-deramond
Copy link
Member

julien-deramond commented Dec 18, 2024

Sorry for the delay, @guilhas07, and thank you for taking the time to work on this PR!
I haven't digged into the history of the implementation of the Scrollspy component yet, nor looked at your code.
Here's my feedback after testing and comparing the behavior with the current implementation of the Scrollspy component.

  • General improvement: Overall, this feels less buggy, as all items are active while scrolling. This is a great enhancement!
  • Behavior on specific pages: On the Reboot documentation page, for example, the last element is active in the current implementation but not in your branch. Personally, I don’t mind this change—it aligns with how some other frameworks (e.g., Astro/Starlight) handle small sections that "stick" to the bottom of the page.
  • In the Scrollspy Navbar example from your branch, selecting an item from the dropdown and then scrolling results in two .active elements simultaneously (which shouldn't happen IMO).

Screenshot 2024-12-18 at 08 53 13

Screenshot 2024-12-18 at 08 55 30

To wrap up, I think this PR has potential, but we need to address these edge cases. Once finalized, it would be great to add strict unit tests to enforce the new behavior. These tests could also act as documentation for the expected behavior, helping prevent regressions in the future.

Edit: be careful, the tests fail right now. You can run npm run js-test locally to replicate the issues.

Anyone interested in this topic, feel free to jump in and share your feedback! (/cc @twbs/js-review, you have more expertise on this than I do).

(Note for us, maintainers: if this PR ends up being the main one to fix Scrollspy issues, let's add the references of all the issues related to the Scrollspy)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants