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

v4 - Move collapse js away from panels to cards #17171

Closed
wants to merge 1 commit into from

Conversation

kkirsche
Copy link
Contributor

Fix #17170

Probably cc fat?

@kkirsche kkirsche changed the title Move collapse js away from panels to cards v4 - Move collapse js away from panels to cards Aug 21, 2015
@mdo mdo added this to the v4.0.0-alpha.2 milestone Sep 2, 2015
@cvrebert
Copy link
Collaborator

cvrebert commented Sep 2, 2015

@kkirsche Could you please rebase this?

@kkirsche
Copy link
Contributor Author

kkirsche commented Sep 2, 2015

@cvrebert rebased

@kkirsche
Copy link
Contributor Author

kkirsche commented Sep 2, 2015

The failure here is unrelated to the change made:

Running "scsslint:src" (scsslint) task
Running scss-lint on src
scss/_reboot.scss:286 [W] PropertySortOrder: Properties should be ordered box-sizing, -webkit-appearance
Warning: Task "scsslint:src" failed.� Use --force to continue.
Aborted due to warnings.

@cvrebert
Copy link
Collaborator

cvrebert commented Sep 2, 2015

@kkirsche × 4 tests failed in "collapse" module when I test it locally.
(There are some other failures, but those are also present in v4-dev 😒 )

@kkirsche
Copy link
Contributor Author

kkirsche commented Sep 2, 2015

@cvrebert noted. I'll look at those probably next week. Traveling for work this week so may be a bit before I can tackle making those fixes

@dalabarge
Copy link

Would it be possible to also have this be a configurable selector using data-selector or something? Having JS rely upon hard-coded HTML class names makes it more difficult to use semantic class names and then use SCSS to mixin the appropriate styles. Just spent an hour or so trying to get this working before reading the source and realizing it was selecting child elements of .panel (now .card) instead of just collapsing the elements matching $(parent).find('.collapse.in'). What's the reason we're sticking to making this plugin dependent upon any specific HTML class name?

@cvrebert
Copy link
Collaborator

@dalabarge That's still on the roadmap. This is just a quick fix until we can get around to implementing that.
See #17021:

Collapse

@fat
Copy link
Member

fat commented Nov 15, 2015

yeah this is great - do you mind just poppin ACTIVES selector onto the default object while you're at it so people can change?

@fat
Copy link
Member

fat commented Nov 15, 2015

(otherwise lgtm)

@mdo mdo modified the milestones: v4.0.0-alpha.2, v4.0.0-alpha.3 Dec 8, 2015
@mdo mdo modified the milestones: v4.0.0-alpha.3, v4.0.0-alpha.4 Jul 23, 2016
@mdo
Copy link
Member

mdo commented Sep 8, 2016

Would love to get this merged into Alpha 5. Was @fat's feedback addressed?

@Johann-S
Copy link
Member

Johann-S commented Sep 8, 2016

My PR #18400 do the same things but is up to date and apply changes to js/tests/visual/modal.html which use Collapse too.

@mdo
Copy link
Member

mdo commented Oct 11, 2016

Merged #18400 to address this. Thanks though @kkirsche!

@mdo mdo closed this Oct 11, 2016
@kkirsche
Copy link
Contributor Author

Course. Thanks for considering this

@kkirsche kkirsche deleted the patch-23 branch October 12, 2016 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants