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

Multi accordion support + unit test #25161

Merged
merged 5 commits into from
Jan 3, 2018

Conversation

MartijnCuppens
Copy link
Member

Fixes #24810.

@Johann-S
Copy link
Member

Johann-S commented Jan 2, 2018

Nice PR @MartijnCuppens I'll take time to review it asap

Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

LGTM from a quick look. @Johann-S please verify when you can.

@Johann-S
Copy link
Member

Johann-S commented Jan 3, 2018

Is it possible to see a CodePen with your feature @MartijnCuppens ? please
And can you update your branch 😉

@MartijnCuppens
Copy link
Member Author

Her you go @Johann-S: https://codepen.io/MartijnCuppens/pen/YYxpqW?editors=1000

Shall I also include an example in the docs?

@Johann-S
Copy link
Member

Johann-S commented Jan 3, 2018

I picked up this code from our documentation and your change break the current behavior see : https://codepen.io/Johann-S/pen/PEKbdR?editors=1000

The example is here in our documentation : https://getbootstrap.com/docs/4.0/components/collapse/#accordion-example
(the second one)

An example or a quick note would be rad 👍

@MartijnCuppens
Copy link
Member Author

@Johann-S , doesn't data-parent need to be set on the element with the .collapse class?

@XhmikosR
Copy link
Member

XhmikosR commented Jan 3, 2018 via email

@Johann-S
Copy link
Member

Johann-S commented Jan 3, 2018

Nope my bad this example is incorrect we should remove that example which isn't relevant anymore #22251 (since we removed the use or data-children)

@MartijnCuppens
Copy link
Member Author

@Johann-S, I already took care of that ;) : https://github.com/twbs/bootstrap/pull/25121/files

@XhmikosR XhmikosR merged commit a1d134f into twbs:v4-dev Jan 3, 2018
@mdo mdo mentioned this pull request Jan 3, 2018
@Johann-S
Copy link
Member

Johann-S commented Jan 3, 2018

Nice thank you @MartijnCuppens 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants