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

Reverse #22138 and detach accordion from card without requiring 'data-children' #22251

Merged
merged 12 commits into from
Apr 25, 2017

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Mar 23, 2017

#22138 was merge in order to detach accordion from .card and is based on a new attribute data-children that require to reference the class of the child.

This PR reverse #22138 and implement a more simple solution that does not require any new attribute.

I initially wanted to give my feedback on the original PR, but somehow forgot and remembered when I saw it merged...

@@ -132,7 +124,7 @@ const Collapse = (($) => {
let activesData

if (this._parent) {
actives = $.makeArray($(this._parent).find(this._selectorActives))
actives = $.makeArray($(this._parent).children().children(Selector.ACTIVES))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is better or not, but you do a double find of children instead of one. So if they are a huge collapse it wont be fast as the previous PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The find() method in JQuery go through the whole tree of child (life child, grand-child, grand-grand-child).
The children() go through only one level. So 2 calls to children() will most likely be faster than find() in case there is content in the .collapse (that would be more than 2 level). Most likely there would be content in the .collapse.

In addition calling children() twice might actually be faster than ${childrenSelector} > .show, ${childrenSelector} > .collapsing as it would loop on the child nodes in the tree rather than calling Sizzle/querySelector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But frankly if there is a performance difference, one way or the other, it would be a matter nano seconds per collapsible. Unless you have an accordion with million of children the difference will be close to nothing.

I think the simplification for theBootstrap user (getting rid of data-children attribute) worth the few nano seconds that might be lost or gained.

@mdo
Copy link
Member

mdo commented Mar 23, 2017

Does any of this business with children and find affect nested accordions? #18632

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 23, 2017

In both cases the selector will select only the direct child the accordion that is collapsible, i.e .card or whatever you use in place of card.
So nested accordion would work independently of each other.

Maybe I can add a unit test for nested accordion. Would you prefer that in this PR or a different one ?

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 23, 2017

Actually forget my previous comment. The current implementation will not work with nested accordion as the selector ${childrenSelector} > .show, ${childrenSelector} > .collapsing will match each nested level (if they the collapsible use the same class passed in data-children), so the parent accordion will interfere with the child.

This PR will incidentally fix this issue as the children() will match only the collapsible directly inside the accordion and not the nested one.

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 23, 2017

@mdo, I added a unit test for nested accordions. It passes with this PR but doesn't in the current v4-dev.
So I can confirm that on the current v4-dev the nested accordion do not work and that this PR "incidentally" fix this issue.

@mdo mdo added this to the v4.0.0-beta milestone Apr 9, 2017
@mdo mdo requested a review from bardiharborow April 9, 2017 06:34
Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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