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: fix wrong activation of all nested links #20304

Merged
merged 2 commits into from
Dec 23, 2016

Conversation

mr-july
Copy link
Contributor

@mr-july mr-july commented Jul 14, 2016

if the scrollspy applied to hierarchical navigation, then too many nav-links are activated.
Example markup:

      <ul id="toc" class="nav nav-pills nav-stacked">
        <li>
          <a href="#v1" class="nav-link">Volume 1</a>
          <ul class="nav">
            <li>
              <a href="#ch-1-1" class="nav-link">Chapter 1.1</a>
            </li>
            <li>
              <a href="#ch-1-2" class="nav-link">Chapter 1.2</a>
            </li>
            <li>
              <a href="#ch-1-3" class="nav-link">Chapter 1.3</a>
            </li>
          </ul>
        </li>
        <li>
          <a href="#v2" class="nav-link">Volume 2</a>
          <ul class="nav">
            <li>
              <a href="#ch-2-1" class="nav-link">Chapter 2.1</a>
            </li>
            <li>
              <a href="#ch-2-2" class="nav-link">Chapter 2.2</a>
            </li>
          </ul>
        </li>
      </ul>

on activation of "Chapter 1.2" all siblings ("Chapter 1.1" and "Chapter 1.3") are also activated, because the selector

$('.nav-link[href="#ch-1-2"]').parents('li').find('.nav-links')

matches all ".nav-link"s under "#v1", so the correct selector should be

$('.nav-link[href="#ch-1-2"]').parents('li').find('> .nav-links')
                                                   ^
                                      only direct children of li

The illustration of this problem can be found on JSFiddle, and the solution (just patched version of bootstrap.min.js) is here.

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)

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

@mr-july
Copy link
Contributor Author

mr-july commented Jul 14, 2016

This is the isolated version of #20209. I hope, that with problem demonstration and solution it will be easy to accept this request. ;-)

@mr-july
Copy link
Contributor Author

mr-july commented Jul 14, 2016

Here is the revised version of problem example and solution.

@el222wg
Copy link

el222wg commented Aug 2, 2016

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.

@mr-july
Copy link
Contributor Author

mr-july commented Aug 9, 2016

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.

@mdo
Copy link
Member

mdo commented Oct 28, 2016

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 .nav-item parent and I don't want to enforce that restriction. This was the only relevant PR that didn't do that.

Copy link
Member

@bardiharborow bardiharborow left a 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.

@bardiharborow
Copy link
Member

@mdo @Johann-S are we good to go on this?

@Johann-S
Copy link
Member

It seems we try to avoid child selector see :
#18400 (comment)

So of it's possible to avoid the child selector in this PR I think it would be better

@mdo
Copy link
Member

mdo commented Dec 23, 2016

@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 :).

@mdo mdo merged commit 1d6cdb6 into twbs:v4-dev Dec 23, 2016
@mdo mdo mentioned this pull request Dec 23, 2016
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.

7 participants