Fix #15354, #15315: Uninitialized collapsibles should not toggle on .collapse('hide')
#15807
Conversation
The unit test would execute `hide` on an already hidden element, and then check if it has height set (which is added by the transition). This only worked previously, because collapse.js erroniuosly executed `toggle` which then trigged `show` instead of `hide`. Now that `hide` is actually executed, no transition happens, so the height value is not set by the animation. Hence in order to test the `hide` animation, the element must first be shown.
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: b13efde (Please note that this is a fully automated comment.) |
|
|
|
@vsn4ik I don't understand that. |
if (option == 'show' || option == 'hide') { ... }equivalent if (/^(show|hide)$/.test(option)) { ... } |
|
only question I would ask @vsn4ik is while that is shorter I question if it sacrifices readability for such a small gain. |
|
Indeed. 👍 for the long version. |
|
@vsn4ik Holy crap what syntax is that? |
|
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test explains it. It's just a regular expression tester Please excuse brevity — Sent from my mobile device.
|
|
Ooooooh it's regex. That makes a lot more sense. |
|
@Nikita240 Can you remove the |
cvrebert
changed the title
toggle on .collapse('hide')toggle on .collapse('hide')
|
@cvrebert Did I do it right? |
|
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: d9981ae (Please note that this is a fully automated comment.) |
|
Not quite, but it's not a big deal. Don't worry about it. CC: @fat for review |
…ixes #15315 Thanks to @peterblazejewicz & @Nikita240 Adds unit tests based on #14417 X-Ref: #14282 Closes #15807
| @@ -37,7 +37,7 @@ $(function () { | |||
| }) | |||
|
|
|||
| test('should hide a collapsed element', function () { | |||
| var $el = $('<div class="collapse"/>').bootstrapCollapse('hide') | |||
| var $el = $('<div class="collapse in"/>').bootstrapCollapse('hide') | |||
There was a problem hiding this comment.
A Collapsible with .collapse.in is in the shown/expanded (i.e. non-collapsed) state, so this change is invalid per the name of the test, AFAICT.
There was a problem hiding this comment.
You're right. I just have to rename the test.
…ixes #15315 Thanks to @peterblazejewicz & @Nikita240 Adds unit tests based on #14417 X-Ref: #14282 Closes #15807
|
Superseded by #16011. |
Fix #15315
Fix #15354
When you do this
You get this
