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

Collapse.js v4 only toggles one target? #17643

Closed
thecristen opened this issue Sep 18, 2015 · 10 comments
Closed

Collapse.js v4 only toggles one target? #17643

thecristen opened this issue Sep 18, 2015 · 10 comments

Comments

@thecristen
Copy link

http://www.bootply.com/1lBIRBwnA0
This example, using Bootstrap v3, properly shows the two hidden rows when clicking the first plus button.

The same markup using the v4 source only toggles the first row.

@twbs-lmvtfy
Copy link

Hi @thecristen!

You appear to have posted a live example (http://s.bootply.com/render/1lBIRBwnA0), which is always a good first step. However, according to the HTML5 validator, your example has some validation errors, which might potentially be causing your issue:

  • line 56, column 11 thru column 175: The aria-controls attribute must point to an element in the same document.

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.)

@cvrebert
Copy link
Collaborator

@fat Is this intentional?

@codeConcussion
Copy link

I also ran into this problem. Here's another short example...
Working in v3
Broken in v4

@codeConcussion
Copy link

Has anyone found any workarounds for this issue?

@maxbeatty
Copy link
Contributor

v3 was selecting all targets where as v4 is selecting only the first target which explains the difference in behavior.

As I started to refactor v4 to act like v3, the decision to only select the first target seems intentional. In v3, the data used to determine whether to toggle or instantiate is based off only the first element ($target.data('bs.collapse') is really doing $target.first().data('bs.collapse')). The state of your first target is going to be used for every target which could introduce problems. With the v4 implementation, that isn't left to chance.

Has anyone found any workarounds for this issue?

You could use the hide.bs.collapse event and some mix of show/hide calls to recreate the behavior in v4.

@codeConcussion
Copy link

codeConcussion commented Aug 5, 2016

The state of your first target is going to be used for every target

@maxbeatty - Maybe I'm misunderstanding, but it seems like this v3 example contradicts what you're saying. There are two divs and their toggle states are opposite one another. Any button click shows the hidden div and hides the visible div.

I can achieve the same thing in v4 but I have to use javascript. It's just really nice to accomplish this without scripting.

@maxbeatty
Copy link
Contributor

I don't have a dog in this fight. I could have misunderstood the code and intentions. I was mainly trying to help with v4 issues 🚂

There are two divs and their toggle states are opposite one another.

Before any clicks, those two divs are plain HTML divs with CSS classes dictating their appearance.

  1. On that first click, the first div is asked for data about 'bs.collapse'. There isn't any because it's new (hasn't been used by collapse yet) so the plugin is called with the click event target's data and each data-target uses those options.
  2. Each div has no existing data for 'bs.collapse' so each is instantiated.
  3. The default is to toggle so at the end of each instantiation, toggle is called
  4. toggle calls hide or show based on the element having the in class which is why your "Delete" button hides and your "Yes/No" buttons show.

If in v3 your second div is already instantiated for some reason or another, it behaves the same as v4 https://jsfiddle.net/bnz8sa2L/1/

My outsider opinion is that it's better to declare the behavior as you need to in v4 than to depend on assumptions about how it'll behave like in v3

@michaelzangl
Copy link

michaelzangl commented Nov 7, 2016

I ran into the same issue. I'd like to toggle all targets and not the first one. This is the whole point in using CSS selectors... This is how I hacked a fix for it:

--- a/public/js/bootstrap.js
+++ b/public/js/bootstrap.js
@@ -1284,7 +1284,10 @@ var Collapse = (function ($) {
         var selector = '[data-toggle="collapse"][data-parent="' + this._config.parent + '"]';

         $(parent).find(selector).each(function (i, element) {
-          _this6._addAriaAndCollapsedClass(Collapse._getTargetFromElement(element), [element]);
+          var element = element;
+          Collapse._getTargetsFromElement(element).each(function() {
+            _this6._addAriaAndCollapsedClass(this, [element]);
+          });
         });

         return parent;
@@ -1305,10 +1308,10 @@ var Collapse = (function ($) {
       // static

     }], [{
-      key: '_getTargetFromElement',
-      value: function _getTargetFromElement(element) {
+      key: '_getTargetsFromElement',
+      value: function _getTargetsFromElement(element) {
         var selector = Util.getSelectorFromElement(element);
-        return selector ? $(selector)[0] : null;
+        return $(selector ? selector : []);
       }
     }, {
       key: '_jQueryInterface',
@@ -1353,11 +1356,14 @@ var Collapse = (function ($) {
   $(document).on(Event.CLICK_DATA_API, Selector.DATA_TOGGLE, function (event) {
     event.preventDefault();

-    var target = Collapse._getTargetFromElement(this);
-    var data = $(target).data(DATA_KEY);
-    var config = data ? 'toggle' : $(this).data();
-
-    Collapse._jQueryInterface.call($(target), config);
+    var $this = $(this);
+    var targets = Collapse._getTargetsFromElement(this);
+    targets.each(function() {
+           var data = $(this).data(DATA_KEY);
+           var config = data ? 'toggle' : $this.data();
+       
+           Collapse._jQueryInterface.call($(this), config);
+    })
   });

   /**

The patch is against the final bootstrap.js file.

@mdo
Copy link
Member

mdo commented Nov 26, 2016

I'd definitely welcome a PR to address this in our next alpha. Having multiple targets trigger the same collapse element would be awesome.

@pvdlg
Copy link
Contributor

pvdlg commented Jan 21, 2017

X-ref #21692

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

No branches or pull requests

8 participants