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

Allow ScrollSpy to work for List Groups #13050

Closed
CyborgMaster opened this issue Mar 13, 2014 · 7 comments
Closed

Allow ScrollSpy to work for List Groups #13050

CyborgMaster opened this issue Mar 13, 2014 · 7 comments

Comments

@CyborgMaster
Copy link

I'm implementing a side navigation bar using a list group with linked items. Looks something like this:

<div class="list-group side-nav">
  <a class="list-group-item" href="#20131023_appointments">
    <i class="fa fa-chevron-right"></i> October 23, 2013
  </a>
  <a class="list-group-item" href="#20131024_appointments">
    <i class="fa fa-chevron-right"></i> October 24, 2013
  </a>
  <a class="list-group-item" href="#20131025_appointments">
    <i class="fa fa-chevron-right"></i> October 25, 2013
  </a>
  <a class="list-group-item" href="#20131026_appointments">
    <i class="fa fa-chevron-right"></i> October 26, 2013
  </a>
</div>

ScrollSpy currently only works with .nav elements containing links inside li elements. I propose we also support list groups. List groups support active links, which makes them a good candidate for navigation elements, therefore ScrollSpy should support them.

I have an idea about how to do this, but it's really rough, so I'm just going to describe it here instead making a full pull request. If people think the idea is good, I'll write it up and create the pull request.

We could either try and detect a list group, or allow passing an option to specify list-group mode. I'm going with the latter.

This would require implementing the selector something like this:

this.selector       = (this.options.target
       || ((href = $(element).attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '')) //strip for ie7
       || '') + (this.options.listGroup ? ' a' : ' .nav li > a')

and the activation code like this:

    var active;
    if (this.options.listGroup) {
      active = $(this.selector);
    } else {
      active = $(this.selector)
        .parentsUntil(this.options.target, '.active');
    }
    active.removeClass('active');

    var selector = this.selector +
        '[data-target="' + target + '"],' +
        this.selector + '[href="' + target + '"]'

    if (this.options.listGroup) {
      active = $(selector);
    } else {
      active = $(selector)
        .parents('li');
    }
    active.addClass('active'); 

Interestingly enough the code is simpler for the list group case.

What does everyone think? Should I flesh this out into a pull request?

@xPaw
Copy link

xPaw commented May 16, 2014

This is a great idea. I wanted to use scrollspy with list-group today, but found out that it wasn't possible.

@fat
Copy link
Member

fat commented May 22, 2014

i wonder if we would break the internet if we changed it from .nav li > a to just a for everyone…

@fat
Copy link
Member

fat commented May 22, 2014

my feedback is: not crazy about adding an option. I do think it would be cool if this worked with list-groups though.

also, would be cool to get a pr from you… :)

my ideal solution would be to come up with something which worked for both scenarios (without breaking backwards compat)… possible?

@CyborgMaster
Copy link
Author

Great to hear from a core team member! My initial question was simply to find out if it was a good idea. Since you like the idea, PR on its way. I'll noodle a bit on how to support both without an option, I bet it could be done with a little inspection of the DOM. Expect a PR in the next couple hours.

@mdo
Copy link
Member

mdo commented Jun 19, 2014

Probably a v4 thing for the scrollspy which simply targets either the immediate children of a particular element (e.g., all .list-group-items in .list-group and all .nav-items in .nav).

@mdo
Copy link
Member

mdo commented Jun 19, 2014

Calling this a nice to have, but not something we'll commit to for v3.x.

@tostasqb
Copy link

+1 for the idea

@twbs twbs locked and limited conversation to collaborators Apr 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants