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

Collapsible parent option is dependent on dom structure #10966

Closed
steodor opened this issue Oct 7, 2013 · 15 comments
Closed

Collapsible parent option is dependent on dom structure #10966

steodor opened this issue Oct 7, 2013 · 15 comments

Comments

@steodor
Copy link

steodor commented Oct 7, 2013

Hi, i found this pull request: #7600

In the latest version i see that the parent option is now dependent on the .panel class, i.e.:

var actives = this.$parent && this.$parent.find('> .panel > .in')

Why don't you just leave it like it was (.collapse.in) or better yet, just:

var actives = this.$parent && this.$parent.find('.in')

Or, i get it if the parent must have a class, to avoid conflicts with other libraries, but can you please explain what's up with depending on dom structure? (the double > in the selector really don't look good to me)

Oh and, the docs are way behind on this one. There's an .accordion-group class mentioned, but not saying how it should be used, and the example doesn't have one at all (yet it works, probably on an old bootstrap version).

So basically, please decide on a structure and update the docs & examples accordingly. If you want i could help you with some of the workload, as you see fit.

Thank you.

@iatek
Copy link
Contributor

iatek commented Oct 17, 2013

+1 for leaving it like it was -- .collapse.in

@meshkoff
Copy link

Great find!

I just upgraded to a new version after discovering that the version I've been using (which I downloaded as soon as bootstrap 3 was released) doesn't properly manage grids (xs would always be stacked even on mobile etc). So after downloading this new version, the grids work fine but the top nav was broken and so was the accordion behavior. I've been using "accordion-group" class to ensure other collapsible closes when the new one opens. So now just need to replace it with "panel" and it works like it used to. So those that used "accordion-group" in the past could just replace it with "panel".

@steodor
Copy link
Author

steodor commented Oct 21, 2013

Well basically i was trying to find out why we need a container such as accordion-group or panel whatsoever. Seems to me that we don't absolutely need it, and in most cases it just adds worthless markup to the page. So, quoting iatek:
+1 for leaving it like it was -- .collapse.in

@meshkoff
Copy link

You needed the accordion-group if you needed no more than one collapse from the group to be expanded at any given time.

@steodor
Copy link
Author

steodor commented Oct 21, 2013

@meshkoff yes, that is what an accordion is, but you shouldn't need 2 containers for it (i.e. both parent and accordion-group or panel). Just parent should suffice. That's all i'm saying.

@steodor
Copy link
Author

steodor commented Oct 21, 2013

@ThinTim Yup, that's exactly what i meant. Thank you :)

@meshkoff
Copy link

Agree

@jDrysdale
Copy link

I also found this issue confusing when trying to imitate the accordion behavior from the Bootstrap documentation's own .bs-sidenav.

.collapse.in better matches the explanation in the docs.

@TheEricMiller
Copy link

+1

@JTLai
Copy link

JTLai commented Nov 30, 2013

+1

@jrust
Copy link

jrust commented Dec 13, 2013

This appears to be fixed by #11191

@fat
Copy link
Member

fat commented Dec 27, 2013

this is the way it is because of nesting accordions shudder… but yes. you do it. Also, you can't just use .in because .in could be anything…

So for now, yes – > Collapsible parent option is dependent on dom structure

@fat fat closed this as completed Dec 27, 2013
@fat
Copy link
Member

fat commented Dec 27, 2013

also, updated the accordion-group class ref we missed. thanks!

@mvz
Copy link

mvz commented Jun 13, 2017

It looks like Bootstrap 4 will keep the requirement of exactly two levels (

actives = $.makeArray($(this._parent).children().children(Selector.ACTIVES))
). Perhaps now would be a good time to revisit this?

@k-funk
Copy link

k-funk commented Aug 4, 2017

+1
This really ought to be in the docs at the very least. Mystery DOM requirements are a nightmare to debug.

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

Successfully merging a pull request may close this issue.

10 participants