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

Adjust for Scrollspy offset when calculating maxScroll. #13589

Merged
merged 2 commits into from
May 23, 2014

Conversation

mrobinet
Copy link

There is a bug in Scrollspy where it will prematurely jump to the last navigation element when using an offset. The larger the offset used, the more prominent the problem becomes.

Here is a JSFiddle that shows the issue:
http://jsfiddle.net/4gnGy/4/

The fix is to add the offset to the maxScroll. All of the offsets created during Scrollspy initialization will be the offset + each element's scrollTop value so maxScroll needs to account for the offset as well.

@cvrebert
Copy link
Collaborator

Is it feasible to add a unit test for this?

@cvrebert cvrebert added the js label May 13, 2014
@mrobinet
Copy link
Author

I will attempt this. Not terribly familiar with Node and grunt, but I'll give it a shot!

@cvrebert
Copy link
Collaborator

You can run the test suite (/js/tests/index.html) from your browser, FYI.

@mrobinet
Copy link
Author

I got it figured out. Thanks. I added a new test for the maxScroll and also fixed the one above it.

I found that the Scrollspy process event handler was not being called until after the test had completed. The 'should only switch active class on current target' test was testing the active state did not change, which would have been always true since the process event handler had not executed yet.

@mrobinet
Copy link
Author

Does everything look good here now? Will this get merged?

@cvrebert
Copy link
Collaborator

All signs are positive. Still need @fat to review this though.

@cvrebert cvrebert added this to the v3.2.0 milestone May 16, 2014
@mrobinet
Copy link
Author

Thanks for the update.

@fat
Copy link
Member

fat commented May 23, 2014

lgtm

@fat
Copy link
Member

fat commented May 23, 2014

thank you very much!

@cvrebert
Copy link
Collaborator

Running Sauce tests: https://travis-ci.org/twbs/bootstrap/jobs/25907931

@cvrebert
Copy link
Collaborator

Sauce tests passed. Merging per @fat's LGTM.

cvrebert added a commit that referenced this pull request May 23, 2014
Adjust for Scrollspy offset when calculating maxScroll.
@cvrebert cvrebert merged commit 475dbe5 into twbs:master May 23, 2014
@mdo mdo mentioned this pull request May 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants