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

Collapse with flex elements #22758

Closed
wants to merge 3 commits into from
Closed

Collapse with flex elements #22758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df1f914
Collapse with flex elements
Johann-S Jun 6, 2017
d15f2cf
Add visual test
Johann-S Jun 6, 2017
05c311c
Add update method for Collapse plugin
Johann-S Jun 6, 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
4 changes: 4 additions & 0 deletions docs/4.0/components/collapse.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,10 @@ Shows a collapsible element. **Returns to the caller before the collapsible elem

Hides a collapsible element. **Returns to the caller before the collapsible element has actually been hidden** (i.e. before the `hidden.bs.collapse` event occurs).

#### `.collapse('update')`

Update a collapsible element if it changed during its lifetime.

### `.collapse('dispose')`

Destroys an element's collapse.
Expand Down
55 changes: 46 additions & 9 deletions js/src/collapse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const Collapse = (() => {

const ClassName = {
SHOW : 'show',
FLEXSHOW : 'flexshow',
COLLAPSE : 'collapse',
COLLAPSING : 'collapsing',
COLLAPSED : 'collapsed'
Expand Down Expand Up @@ -87,7 +88,8 @@ const Collapse = (() => {
}
}

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

if (!this._config.parent) {
this._addAriaAndCollapsedClass(this._element, this._triggerArray)
Expand All @@ -113,7 +115,7 @@ const Collapse = (() => {
// public

toggle() {
if ($(this._element).hasClass(ClassName.SHOW)) {
if ($(this._element).hasClass(this._showClass)) {
this.hide()
} else {
this.show()
Expand All @@ -122,7 +124,7 @@ const Collapse = (() => {

show() {
if (this._isTransitioning ||
$(this._element).hasClass(ClassName.SHOW)) {
$(this._element).hasClass(this._showClass)) {
return
}

Expand Down Expand Up @@ -176,7 +178,7 @@ const Collapse = (() => {
$(this._element)
.removeClass(ClassName.COLLAPSING)
.addClass(ClassName.COLLAPSE)
.addClass(ClassName.SHOW)
.addClass(this._showClass)

this._element.style[dimension] = ''

Expand All @@ -202,7 +204,7 @@ const Collapse = (() => {

hide() {
if (this._isTransitioning ||
!$(this._element).hasClass(ClassName.SHOW)) {
!$(this._element).hasClass(this._showClass)) {
return
}

Expand All @@ -212,16 +214,15 @@ const Collapse = (() => {
return
}

const dimension = this._getDimension()

const dimension = this._getDimension()
this._element.style[dimension] = `${this._element.getBoundingClientRect()[dimension]}px`

Util.reflow(this._element)

$(this._element)
.addClass(ClassName.COLLAPSING)
.removeClass(ClassName.COLLAPSE)
.removeClass(ClassName.SHOW)
.removeClass(this._showClass)

if (this._triggerArray.length) {
for (let i = 0; i < this._triggerArray.length; i++) {
Expand Down Expand Up @@ -263,6 +264,16 @@ const Collapse = (() => {
this._isTransitioning = isTransitioning
}

update() {
if ($(this._element).hasClass(this._showClass)) {
$(this._element).removeClass(this._showClass)
this._showClass = this._getShowClass()
$(this._element).addClass(this._showClass)
} else {
this._showClass = this._getShowClass()
}
}

dispose() {
$.removeData(this._element, DATA_KEY)

Expand Down Expand Up @@ -316,7 +327,7 @@ const Collapse = (() => {

_addAriaAndCollapsedClass(element, triggerArray) {
if (element) {
const isOpen = $(element).hasClass(ClassName.SHOW)
const isOpen = $(element).hasClass(ClassName.SHOW) || $(element).hasClass(ClassName.FLEXSHOW)

if (triggerArray.length) {
$(triggerArray)
Expand All @@ -326,6 +337,32 @@ const Collapse = (() => {
}
}

_getShowClass() {
const tabClass = this._element.classList

// Detect flex for inline style
let useFlex = $(this._element).css('display').indexOf('flex') !== -1
Copy link
Member

Choose a reason for hiding this comment

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

How about just using https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/includes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched to includes but they are no support on IE 10 so back to indexOf

Copy link
Member

Choose a reason for hiding this comment

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

But it's transpiled isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't checked but unit tests failed

Copy link
Member

Choose a reason for hiding this comment

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

But since we use babel it should be taken care of. Please try one more time.

Copy link
Member

Choose a reason for hiding this comment

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

Just keep in mind we can do this if we just load the babel polyfill and use ES6 methods. Not sure what the exact size diff is, just thinking out loud for another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know which polyfill to use ? I can make a test 👍

Copy link
Member

Choose a reason for hiding this comment

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

https://babeljs.io/docs/usage/polyfill/

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked babel-polyfill and currently when we import babel-polyfill we will import all the available polyfill not the includes one so for now we should merge this PR and open an issue on how to integrate babel-polyfill with only specific es6 polyfill

Copy link
Member

Choose a reason for hiding this comment

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

No worries, it's not so important currently as long as we don't use features that aren't available for the browsers we support.


// Create a wrapper to hide our flex detection
Copy link
Member

@glebm glebm Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can you check getComputedStyle(this._element).display instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can use that for the line 316 yes, but not for the detection below because getComputedStyle return the current applied style and that's not what I want here.
I want to know if they are a class which contain a flex display

const $tmpWrapper = $('<div class="d-none"></div>')
$tmpWrapper.insertAfter($(this._element))

const $tmpElem = $('<div></div>')
$tmpWrapper.append($tmpElem)

// Detect flex in used classes
for (let i = 0; i < tabClass.length; i++) {
$tmpElem.addClass(tabClass[i])
const tmpDisplay = $tmpElem.css('display')
$tmpElem.removeClass(tabClass[i])
if (tmpDisplay.indexOf('flex') !== -1) {
useFlex = true
break
}
}
$tmpWrapper.remove()
return !useFlex ? ClassName.SHOW : ClassName.FLEXSHOW
}

// static

Expand Down
16 changes: 16 additions & 0 deletions js/tests/visual/collapse.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,22 @@ <h5 class="mb-0">
</div>
</div>
</div>
<div class="row mt-2">
<div class="col-sm-12">
<p>
<button class="btn btn-primary" type="button" data-toggle="collapse" data-target="#collapseExample" aria-expanded="false" aria-controls="collapseExample">
Show collapse element with flex
</button>
</p>
<div class="collapse row" id="collapseExample">
<div class="col-sm-5">
<div class="card card-block">
Anim pariatur cliche reprehenderit, enim eiusmod high life accusamus terry richardson ad squid. Nihil anim keffiyeh helvetica, craft beer labore wes anderson cred nesciunt sapiente ea proident.
</div>
</div>
</div>
</div>
</div>
</div>

<script src="../../../assets/js/vendor/jquery-slim.min.js"></script>
Expand Down
9 changes: 7 additions & 2 deletions scss/_transitions.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,21 @@
&.show {
display: block;
}
&.flexshow {
display: flex;
}
}

tr {
&.collapse.show {
&.collapse.show,
&.collapse.flexshow {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should .flexshow be named .flex-show to be consistent with the hyphen separation used by the other .flex-* classes?

display: table-row;
}
}

tbody {
&.collapse.show {
&.collapse.show,
&.collapse.flexshow {
display: table-row-group;
}
}
Expand Down