collapse use on flex rows doesn't restore display property correctly when expanded

[lvmajor]

Okay so the title may not be as clear as it could but here's the problem description:

1. When using flex mode, applying collapse to a row makes it use "display: none" when "collapsed". The problem is when expanding the row (getting .collapse .in selector), the display property is reset to "display: block", which should be "display: flex" to restore normal behavior.

See this problem on codeply: http://codeply.com/go/8WVm4PQBhF

Operating system: Windows 10
Browsers: Chrome 52.0.2743.82, Edge 25.10586.0.0

I'm not sure if this is intended or if it's a real issue so I'm awaiting your feedback before submitting a PR for this.

[judetucker]

Hi @os1r1s110

I had a look at the code that you posted. I've found that it's usually best to wrap the entire element that needs to be collapsed in a dedicated collapse div. This prevents class-crossing issues like the one that you exposed.

Here is code that should make this work for you as you intend. Let me know if this solves your issue. https://codepen.io/judetucker/pen/RRYxzj

[twbs-lmvtfy]

Hi @judetucker!

You appear to have posted a live example (https://codepen.io/judetucker/pen/RRYxzj.html), which is always a good first step. However, according to Bootlint, your example has some Bootstrap usage errors, which might potentially be causing your issue:

• line 15, column 3: E004: Containers (.container and .container-fluid) are not nestable
• line 18, column 9: E013: Only columns (.col-*-*) may be children of .rows
• line 21, column 9: E013: Only columns (.col-*-*) may be children of .rows
• line 27, column 9: E013: Only columns (.col-*-*) may be children of .rows
• line 30, column 9: E013: Only columns (.col-*-*) may be children of .rows

You'll need to fix these errors and post a revised example before we can proceed further.
Thanks!

(Please note that this is a fully automated comment.)

[lvmajor]

Hi @judetucker, this effectively seems to fix the issue. The only thing is I don't know if we should correct it in another way to take into account those who might use rows in a non-dedicated way...

I never came across a documentation part which would mention that rows must be in a dedicated div, but that might be the case and if it is, then I'll be pleased to follow this guideline in the future. Else it could be good to specify that if not used separately from other classes, it may cause issues.

The solution I came up with was to reset style correctly to "display:flex" instead of "display:block" in _animation.scss, line ~10, changing
`.collapse {
display: none;

&.in {
    display: block;
}
// tr&.in    { display: table-row; }
// tbody&.in { display: table-row-group; }

}`

to:
`.collapse {
display: none;

&.in {
    &.row {
        @if $enable-flex {
            display: flex;
        }
        @else {
            display: block;
        }
    }
    display:block;
}
// tr&.in    { display: table-row; }
// tbody&.in { display: table-row-group; }

}`

[lvmajor]

Don't know why the code doesn't display correctly.... the quote is misplaced or what?

[judetucker]

@os1r1s110 it is definitely best practice for simplicity / clean code sake. I do believe that your proposed solution would work, but it might be too much of an edge case to merge into the project.

I'll defer to the core team on that one. Best of luck.

[lvmajor]

@judetucker No problem if it's considered too edgy then I'll just use rows in separate divs and call it a day :P

But I think it could/should be noted somewhere in the docs to make it clear that if rows are not used separately, it might cause weird issues as this one.

[lvmajor]

Just to know, why putting the js label on it? Wouldn't it be easier to make the fix in css?

Or you prefer correcting the js to change class for flex vs standard display mode?

[mdo]

JS label indicates one or more of our JS plugins are involved—the collapse plugin, in this case.

I don't think I'm going to change the block to flex yet, despite us going all in on flexbox these last couple weeks. I'd rather wait and see what folks run into before arbitrarily setting everything to display: flex on folks.