-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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: fix wrong activation of all nested links #20304
Conversation
just first level item should be activated
@@ -252,7 +252,7 @@ const ScrollSpy = (($) => { | |||
} else { | |||
// todo (fat) this is kinda sus... | |||
// recursively add actives to tested nav-links | |||
$link.parents(Selector.LI).find(Selector.NAV_LINKS).addClass(ClassName.ACTIVE) | |||
$link.parents(Selector.LI).find('> ' + Selector.NAV_LINKS).addClass(ClassName.ACTIVE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected string concatenation prefer-template
This is the isolated version of #20209. I hope, that with problem demonstration and solution it will be easy to accept this request. ;-) |
Here is the revised version of problem example and solution. |
Sorry for asking but is this fixed yet? It would be nice if nested lists/navs worked with scrollspy, I think it worked with bootstrap 3. |
emmalovgren, if this pull request will be accepted (I dont't know why it's not accepted yet), then nested lists/navs will be functional with scrollspy. If you don't like to wait, you can create custom build for bootstrap4 and include proposed patch. |
I would love to get this fixed. @bardiharborow I saw you comment on a few related issues weeks back. Any feedback on this? I've closed some of those other issues since they all involve requiring the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing test coverage, but this looks all good from my end.
It seems we try to avoid child selector see : So of it's possible to avoid the child selector in this PR I think it would be better |
@Johann-S For our CSS, yes, we're actively avoiding those except where they are purposefully limiting CSS inheritance. For this, I'm unsure what else we'd do here, so I'm inclined to merge. Should you or anyone else have something else in mind, I'm always open to it :). |
if the scrollspy applied to hierarchical navigation, then too many nav-links are activated.
Example markup:
on activation of "Chapter 1.2" all siblings ("Chapter 1.1" and "Chapter 1.3") are also activated, because the selector
matches all ".nav-link"s under "#v1", so the correct selector should be
The illustration of this problem can be found on JSFiddle, and the solution (just patched version of bootstrap.min.js) is here.