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

Clear scrollspy selection above first section #13563

Closed

Conversation

tadeuzagallo
Copy link
Contributor

Once scrollspy had activated any item of the navbar it wouldn't clear it
anymore, leaving the first item activated for a scrolltop above the
first section.

I don't know if this was intended, but it has been bothering me in every project that I used ScrollSpy.

@cvrebert cvrebert added the js label May 11, 2014
@cvrebert cvrebert added this to the v3.2.1 milestone May 23, 2014
@cvrebert
Copy link
Collaborator

@tadeuzagallo Could you please remove /dist/js/bootstrap.min.js from this PR? It prevents clean merges.

@tadeuzagallo
Copy link
Contributor Author

Oh, sorry... Just removed it!

return activeTarget != (i = targets[0]) && this.activate(i)
if (activeTarget && scrollTop < offsets[0]) {
this.activeTarget = null
return this.clear()
Copy link
Collaborator

Choose a reason for hiding this comment

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

But clear() doesn't return anything...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just to stop the script... Should I change it to

this.clear()
return

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see, this is Jacob being idiosyncratic in his style. Nevermind then. Leave as-is for the time being.

@cvrebert
Copy link
Collaborator

cvrebert commented Aug 6, 2014

/cc @fat for review

@hnrch02
Copy link
Collaborator

hnrch02 commented Sep 17, 2014

@tadeuzagallo Could you write a unit test for this to prevent future regression?

@cvrebert cvrebert mentioned this pull request Oct 26, 2014
@hnrch02 hnrch02 closed this in e8af21c Oct 26, 2014
@hnrch02
Copy link
Collaborator

hnrch02 commented Oct 26, 2014

Thanks for the work! I merged a modified version with a unit test.

@tadeuzagallo
Copy link
Contributor Author

Oh, I'm sorry, I hadn't seen the comment regarding the tests...
Anyway, I'm glad to be able to help, even if it's a small bit.

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