Fix #15741: Update all collapse triggers with collapsed class and aria-expanded #15751
Conversation
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 73e5911 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 96a0230 (Please note that this is a fully automated comment.) |
|
Trigger option was introduced in #14686, and its removal does not break that fix afaik. |
|
/cc @fat for review |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 4dd7c0b (Please note that this is a fully automated comment.) |
cvrebert
changed the title
|
@cvrebert any update on this XD |
|
Hmm, |
| @@ -16,7 +16,7 @@ | |||
| var Collapse = function (element, options) { | |||
| this.$element = $(element) | |||
| this.options = $.extend({}, Collapse.DEFAULTS, options) | |||
| this.$trigger = $(this.options.trigger).filter('[href="#' + element.id + '"], [data-target="#' + element.id + '"]') | |||
| this.$trigger = $('[data-toggle="collapse"]').filter('[href="#' + element.id + '"], [data-target="#' + element.id + '"]') | |||
There was a problem hiding this comment.
Using a slightly more complex selector and getting rid of the filter intuitively seems like it would be faster, although I haven't microbenchmarked it.
There was a problem hiding this comment.
But it would need to be '[data-toggle="collapse"][href="' + element.id '"], [data-toggle="collapse"][data-target="#' + element.id + '"]' which is longer.
|
The reason I introduced it was so I could pass the trigger element along directly but this change makes sense. |
| @@ -80,7 +80,7 @@ $(function () { | |||
| $('<div id="test1"/>') | |||
| .appendTo('#qunit-fixture') | |||
| .on('shown.bs.collapse', function () { | |||
| ok(!$target.hasClass('collapsed')) | |||
| ok(!$target.hasClass('collapsed'), 'target does not have class collapsed') | |||
There was a problem hiding this comment.
"target does not have collapsed class"
|
Needs a rebase and after fixing up those nitpicks |
|
Superseded by #15941, which applies the feedback from the code review and has been rebased. |
Fixes #15741. I removed the trigger option. Please let me know if it actually does something.