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

Revamp Scrollspy using Intersection observer #33421

Merged
merged 22 commits into from
Apr 13, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Mar 22, 2021

Based on #33015.

Seems operational on the preview scrollspy page.

Needs:

  • Documentation
  • tweak tests

Advantages:

  • No need of relative/overflowing wrappers etc
  • Keeps offset config as deprecated, for backwards compatibility
  • Emits "activate" events in the same element that was initialized on, discarding the previous incompatibility where in case of body initialization, event was emitting to window

Disadvantages:

  • The method option is ignored now
  • all first observables, get the active class on first render

closes #35727
closes #33943
closes #28638
closes #35796
closes #28807
may fix #35628

@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from 9d2bf55 to f72963e Compare March 24, 2021 00:08
@XhmikosR XhmikosR changed the title Revamp Scrollspy usin Intersection observer Revamp Scrollspy using Intersection observer Mar 24, 2021
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from f72963e to cc02ab0 Compare April 3, 2021 22:17
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from cc02ab0 to f397a43 Compare June 4, 2021 10:12
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from f397a43 to d3fb671 Compare June 14, 2021 23:35
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from d3fb671 to 668d429 Compare June 23, 2021 15:24
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch 2 times, most recently from 48395cf to 27bef39 Compare June 25, 2021 18:14
@GeoSot GeoSot marked this pull request as ready for review June 25, 2021 18:33
@GeoSot GeoSot requested a review from a team as a code owner June 25, 2021 18:33
@GeoSot
Copy link
Member Author

GeoSot commented Jun 25, 2021

After three months, I think, I have something to show 😃

@XhmikosR
Copy link
Member

It seems there's some funky thing going on in https://deploy-preview-33421--twbs-bootstrap.netlify.app/docs/5.0/components/scrollspy/#example-with-nested-nav item 3.1: if I scroll to 3.2 and then back up, 3.1 isn't highlighted.

It might need some tweaking with the stepping or whatever it's called :) Also, not sure what the TODO means? If the PR isn't ready yet, let's mark it as draft.

@GeoSot
Copy link
Member Author

GeoSot commented Jun 29, 2021

Unfortunately it is the same problem as now. Two visible elements in wrapper. Only the upper can be highlighted.
Tried some tricks, like visible percentage, but would lead to other problems if par example the element is higher than its overflowing wrapper 😞

@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from 27bef39 to a0a8d25 Compare July 9, 2021 13:08
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

On the feature side, it does not highlight exactly at the same moment than our previous version, but this one feels more natural to me.

Regarding the nested items talked about previously, I can confirm it already does the same in our current release and even worse: this PR does highlight the 3-1 item very quickly, what our current release doesn't do.

Si I'd go for it :)

@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from a0a8d25 to c8a55dc Compare July 20, 2021 11:05
@XhmikosR XhmikosR added this to In progress in v5.2.0 via automation Jul 29, 2021
@twbs twbs deleted a comment from SangKaaa Aug 14, 2021
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from e110cda to 3088f04 Compare September 8, 2021 23:24
@GeoSot GeoSot requested a review from a team as a code owner September 8, 2021 23:24
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from b1d4652 to 561d11d Compare September 18, 2021 00:35
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch from 561d11d to c538d0c Compare September 28, 2021 22:30
@GeoSot GeoSot marked this pull request as draft October 4, 2021 15:09
@GeoSot GeoSot force-pushed the gs-scrollspy-wth-intersection_observer branch 2 times, most recently from 85d6504 to 14a00a8 Compare October 5, 2021 23:20
@XhmikosR XhmikosR moved this from In progress to Review in progress in v5.2.0 Dec 1, 2021
@mdo mdo force-pushed the gs-scrollspy-wth-intersection_observer branch from 1f09535 to 5662ac4 Compare April 13, 2022 16:59
@mdo mdo merged commit ece1601 into main Apr 13, 2022
v5.2.0 automation moved this from Reviewer approved to Done Apr 13, 2022
@mdo mdo deleted the gs-scrollspy-wth-intersection_observer branch April 13, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.2.0
  
Done
6 participants