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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Apr 25, 2017
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f12a613
Reverse 099486f
pvdlg Mar 23, 2017
77bccf7
Reverse fa1504e
pvdlg Mar 23, 2017
4f37ad2
Revert "Add unit test"
pvdlg Mar 23, 2017
ed6fd20
Reverse #22138 and detache accordion from card without requiring 'dat…
pvdlg Mar 23, 2017
243f253
Also test closing the second item in the accordion
pvdlg Mar 23, 2017
712c09f
Add unit test for nested accordion
pvdlg Mar 23, 2017
9e5ed39
Merge branch 'v4-dev' into fix-collapse-no-card
pvdlg Mar 30, 2017
dd30c4d
Merge branch 'v4-dev' into fix-collapse-no-card
pvdlg Mar 31, 2017
489d8fc
Merge branch 'v4-dev' into fix-collapse-no-card
pvdlg Apr 10, 2017
af6c9ee
Merge branch 'v4-dev' into fix-collapse-no-card
pvdlg Apr 12, 2017
276018c
Merge branch 'v4-dev' into fix-collapse-no-card
pvdlg Apr 14, 2017
253d6e0
Merge branch 'v4-dev' into fix-collapse-no-card
Johann-S Apr 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions js/src/collapse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,8 @@ const Collapse = (($) => {
}

const Selector = {
ACTIVES : '.card > .show, .card > .collapsing',
DATA_TOGGLE : '[data-toggle="collapse"]',
DATA_CHILDREN : 'data-children'
ACTIVES : '.show, .collapsing',
DATA_TOGGLE : '[data-toggle="collapse"]'
}


Expand All @@ -78,20 +77,13 @@ const Collapse = (($) => {
`[data-toggle="collapse"][href="#${element.id}"],` +
`[data-toggle="collapse"][data-target="#${element.id}"]`
))

this._parent = this._config.parent ? this._getParent() : null

if (!this._config.parent) {
this._addAriaAndCollapsedClass(this._element, this._triggerArray)
}

this._selectorActives = Selector.ACTIVES
if (this._parent) {
const childrenSelector = this._parent.hasAttribute(Selector.DATA_CHILDREN) ? this._parent.getAttribute(Selector.DATA_CHILDREN) : null
if (childrenSelector !== null) {
this._selectorActives = `${childrenSelector} > .show, ${childrenSelector} > .collapsing`
}
}

if (this._config.toggle) {
this.toggle()
}
Expand Down Expand Up @@ -129,7 +121,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.

if (!actives.length) {
actives = null
}
Expand Down
76 changes: 67 additions & 9 deletions js/tests/unit/collapse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,27 +491,85 @@ $(function () {
})

QUnit.test('should allow accordion to use children other than card', function (assert) {
assert.expect(2)
assert.expect(4)
var done = assert.async()
var accordionHTML = '<div id="accordion" data-children=".item">'
var accordionHTML = '<div id="accordion">'
+ '<div class="item">'
+ '<a id="linkTrigger" data-parent="#accordion" data-toggle="collapse" href="#collapseOne" aria-expanded="false" aria-controls="collapseOne"></a>'
+ '<div id="collapseOne" class="collapse" role="tabpanel" aria-labelledby="headingThree"></div>'
+ '</div>'
+ '<div class="item">'
+ '<a data-toggle="collapse" data-parent="#accordion" href="#collapseTwo" aria-expanded="false" aria-controls="collapseTwo"></a>'
+ '<a id="linkTriggerTwo" data-toggle="collapse" data-parent="#accordion" href="#collapseTwo" aria-expanded="false" aria-controls="collapseTwo"></a>'
+ '<div id="collapseTwo" class="collapse show" role="tabpanel" aria-labelledby="headingTwo"></div>'
+ '</div>'
+ '</div>'

$(accordionHTML).appendTo('#qunit-fixture')
var $target = $('#linkTrigger')
$('#collapseOne').on('shown.bs.collapse', function () {
assert.ok($(this).hasClass('show'))
assert.ok(!$('#collapseTwo').hasClass('show'))
done()
var $trigger = $('#linkTrigger')
var $triggerTwo = $('#linkTriggerTwo')
var $collapseOne = $('#collapseOne')
var $collapseTwo = $('#collapseTwo')
$collapseOne.on('shown.bs.collapse', function () {
assert.ok($collapseOne.hasClass('show'), '#collapseOne is shown')
assert.ok(!$collapseTwo.hasClass('show'), '#collapseTwo is not shown')
$collapseTwo.on('shown.bs.collapse', function () {
assert.ok(!$collapseOne.hasClass('show'), '#collapseOne is not shown')
assert.ok($collapseTwo.hasClass('show'), '#collapseTwo is shown')
done()
})
$triggerTwo.trigger($.Event('click'))
})
$target.trigger($.Event('click'))
$trigger.trigger($.Event('click'))
})

QUnit.test('should collapse accordion children but not nested accordion children', function (assert) {
assert.expect(9)
var done = assert.async()
$('<div id="accordion">'
+ '<div class="item">'
+ '<a id="linkTrigger" data-parent="#accordion" data-toggle="collapse" href="#collapseOne" aria-expanded="false" aria-controls="collapseOne"></a>'
+ '<div id="collapseOne" class="collapse" role="tabpanel" aria-labelledby="headingThree">'
+ '<div id="nestedAccordion">'
+ '<div class="item">'
+ '<a id="nestedLinkTrigger" data-parent="#nestedAccordion" data-toggle="collapse" href="#nestedCollapseOne" aria-expanded="false" aria-controls="nestedCollapseOne"></a>'
+ '<div id="nestedCollapseOne" class="collapse" role="tabpanel" aria-labelledby="headingThree">'
+ '</div>'
+ '</div>'
+ '</div>'
+ '</div>'
+ '</div>'
+ '<div class="item">'
+ '<a id="linkTriggerTwo" data-toggle="collapse" data-parent="#accordion" href="#collapseTwo" aria-expanded="false" aria-controls="collapseTwo"></a>'
+ '<div id="collapseTwo" class="collapse show" role="tabpanel" aria-labelledby="headingTwo"></div>'
+ '</div>'
+ '</div>').appendTo('#qunit-fixture')
var $trigger = $('#linkTrigger')
var $triggerTwo = $('#linkTriggerTwo')
var $nestedTrigger = $('#nestedLinkTrigger')
var $collapseOne = $('#collapseOne')
var $collapseTwo = $('#collapseTwo')
var $nestedCollapseOne = $('#nestedCollapseOne')


$collapseOne.one('shown.bs.collapse', function () {
assert.ok($collapseOne.hasClass('show'), '#collapseOne is shown')
assert.ok(!$collapseTwo.hasClass('show'), '#collapseTwo is not shown')
assert.ok(!$('#nestedCollapseOne').hasClass('show'), '#nestedCollapseOne is not shown')
$nestedCollapseOne.one('shown.bs.collapse', function () {
assert.ok($collapseOne.hasClass('show'), '#collapseOne is shown')
assert.ok(!$collapseTwo.hasClass('show'), '#collapseTwo is not shown')
assert.ok($nestedCollapseOne.hasClass('show'), '#nestedCollapseOne is shown')
$collapseTwo.one('shown.bs.collapse', function () {
assert.ok(!$collapseOne.hasClass('show'), '#collapseOne is not shown')
assert.ok($collapseTwo.hasClass('show'), '#collapseTwo is shown')
assert.ok($nestedCollapseOne.hasClass('show'), '#nestedCollapseOne is shown')
done()
})
$triggerTwo.trigger($.Event('click'))
})
$nestedTrigger.trigger($.Event('click'))
})
$trigger.trigger($.Event('click'))
})

QUnit.test('should not prevent event for input', function (assert) {
Expand Down