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.

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

Add horizontal collapse support #34253

Merged
merged 1 commit into from Jul 6, 2021
Merged

Add horizontal collapse support #34253

merged 1 commit into from Jul 6, 2021

Conversation

mdo
Copy link
Member

@mdo mdo commented Jun 14, 2021

Thanks @vanillajonathan for the demo at #17496!

This PR finally implements this longstanding request for a horizontal collapse. We already have the JS support, so we were missing the CSS for .width.collapsing and a child element with a fixed width.

Fixes #17496.

Preview: https://deploy-preview-34253--twbs-bootstrap.netlify.app/docs/5.0/components/collapse/#horizontal

@mdo mdo added this to In progress in v4.6.2 via automation Jun 14, 2021
@mdo mdo added this to Inbox in v5.1.0 via automation Jun 14, 2021
@mdo mdo requested a review from a team as a code owner June 14, 2021 20:26
@GeoSot
Copy link
Member

GeoSot commented Jun 14, 2021

As .width is very generic as class name, maybe we can use a more scoped class ex: collapse-horizontal etc?
If I am not wrong, is about one line change on js code (collapse.js line:269)

@mdo
Copy link
Member Author

mdo commented Jun 14, 2021

I think since folks may have been using this already (in both v4 and v5), we're stuck with .width. I've searched around and this class has come up a couple times. I agree though and would love to change it in v6.

@voltaek
Copy link
Contributor

voltaek commented Jun 15, 2021

From what I can see, .width isn't in Bootstrap v4 or v5, so why do we need to support a markedly bad class name when it's never been in the codebase? Your other attempt at implementing this was already using .collapse-horizontal, anyways.

@mdo
Copy link
Member Author

mdo commented Jun 15, 2021

These lines are what makes .width functional right now:

const WIDTH = 'width'

_getDimension() {
return this._element.classList.contains(WIDTH) ? WIDTH : HEIGHT
}

Switching to any other class would require changing some JS. Arguably no one should be using .width because it's completely undocumented and unsupported, so I'm in favor of changing it, but wanted to raise that it could be a breaking change to someone out there.

@alpadev
Copy link
Contributor

alpadev commented Jun 15, 2021

cough #31681 (comment) Not sure if I should feel ignored.. :P

@mdo
Copy link
Member Author

mdo commented Jun 15, 2021

cough #31681 (comment) Not sure if I should feel ignored.. :P

Lol, nooo, I feel so bad. Sorry for not seeing that! 😅

@mdo mdo requested a review from a team as a code owner June 23, 2021 05:36
@mdo
Copy link
Member Author

mdo commented Jun 23, 2021

Renamed the class from .width to .collapse-horizontal.

Note that if we want to backport this to v4, we'd likely have to use .width still as the examples I found used that class and it's so far along in v4's life that we wouldn't want to change IMO. Still not 100% for sure we should change it here even though it's not the best :).

@mdo mdo moved this from Inbox to Review in v5.1.0 Jun 24, 2021
js/src/collapse.js Outdated Show resolved Hide resolved
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Very elegant, a great example of collaborative work! ❤️

@@ -40,6 +40,29 @@ Generally, we recommend using a button with the `data-bs-target` attribute. Whil
</div>
{{< /example >}}

## Horizontal

The collapse plugin also supports horizontal collapsing. Add the `.collapse-horizontal` modifier class to transition the `width` instead of `height` and set a `width` on the immediate child element.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe promote our width utilities here, and a link to how-to add utilities?

The collapse plugin also supports horizontal collapsing. Add the `.collapse-horizontal` modifier class to transition the `width` instead of `height` and set a `width` on the immediate child element.

{{< callout info >}}
Please note that while the example below has a `min-height` set to avoid excessive repaints in our docs, this is not explicitly required. **Only the `width` on the child element is required.**
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if contains and will-change could help here? Can't play with this right now, but something to keep in mind for improving v5 regarding repaint / reflow and animation.

v5.1.0 automation moved this from Review to Approved Jun 29, 2021
@mdo mdo merged commit 359ed09 into main Jul 6, 2021
v5.1.0 automation moved this from Approved to Done Jul 6, 2021
@mdo mdo deleted the collapse-width branch July 6, 2021 03:22
v4.6.2 automation moved this from In progress to Done Jul 6, 2021
@XhmikosR XhmikosR moved this from Done to Needs manual backport in v4.6.2 Nov 3, 2021
@mdo mdo mentioned this pull request May 24, 2022
@mdo mdo moved this from Needs manual backport to Done in v4.6.2 May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.6.2
  
Done
v5.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

Horizontal collapse
5 participants