Pass data arguments to modal? #531

Closed
andriijas opened this Issue Nov 3, 2011 · 48 comments

Comments

Projects
None yet
@andriijas
Contributor

andriijas commented Nov 3, 2011

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Nov 3, 2011

Member

$('[data-controls-modal="countryModal"]').data().country

Member

fat commented Nov 3, 2011

$('[data-controls-modal="countryModal"]').data().country

@fat fat closed this Nov 3, 2011

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Nov 4, 2011

Contributor

That doesnt work when I have multiple lins using data-controls-modal="countryModal"

I know I can bind click event on my own but would just be insanely nice not to. :-)

Contributor

andriijas commented Nov 4, 2011

That doesnt work when I have multiple lins using data-controls-modal="countryModal"

I know I can bind click event on my own but would just be insanely nice not to. :-)

@fat

This comment has been minimized.

Show comment
Hide comment
@fat

fat Nov 4, 2011

Member

ahh interesting... well I think for now we aren't going to pass the data options around for you, as i don't think this is a very common thing. sorry about that :(

Member

fat commented Nov 4, 2011

ahh interesting... well I think for now we aren't going to pass the data options around for you, as i don't think this is a very common thing. sorry about that :(

@fl00r

This comment has been minimized.

Show comment
Hide comment
@fl00r

fl00r Mar 29, 2012

Common way is to use one modal for page.

Then, by clicking one of the links modal appears. So we should know which link shows modal to load information into it.

In my case I have got a row of items and a "show log" button for each. When I click "show log" modal appears but I need to understand what particular log to load.

fl00r commented Mar 29, 2012

Common way is to use one modal for page.

Then, by clicking one of the links modal appears. So we should know which link shows modal to load information into it.

In my case I have got a row of items and a "show log" button for each. When I click "show log" modal appears but I need to understand what particular log to load.

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Mar 29, 2012

Contributor

@fl00r i agree it would be nice to know the origin of the show event, in the same way we know previous and next tab in tabbables. Im going to check the code and see if I can submit a pull request.

Contributor

andriijas commented Mar 29, 2012

@fl00r i agree it would be nice to know the origin of the show event, in the same way we know previous and next tab in tabbables. Im going to check the code and see if I can submit a pull request.

@jimryan

This comment has been minimized.

Show comment
Hide comment
@jimryan

jimryan Apr 1, 2012

+1 for this. Consider a case where multiple elements trigger the same modal with a slight variation in state (like a single attribute difference, or so).

@andriijas Have you made any progress on this?

jimryan commented Apr 1, 2012

+1 for this. Consider a case where multiple elements trigger the same modal with a slight variation in state (like a single attribute difference, or so).

@andriijas Have you made any progress on this?

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Apr 2, 2012

Contributor

Not yet. I Had an AFK weekend.

Sent from my frank phone, mhm

On Sunday 1 April 2012 at 21:20, Jim Ryan wrote:

+1 for this. Consider a case where multiple elements trigger the same modal with a slight variation in state (like a single attribute difference, or so).

@andriijas Have you made any progress on this?


Reply to this email directly or view it on GitHub:
twitter#531 (comment)

Contributor

andriijas commented Apr 2, 2012

Not yet. I Had an AFK weekend.

Sent from my frank phone, mhm

On Sunday 1 April 2012 at 21:20, Jim Ryan wrote:

+1 for this. Consider a case where multiple elements trigger the same modal with a slight variation in state (like a single attribute difference, or so).

@andriijas Have you made any progress on this?


Reply to this email directly or view it on GitHub:
twitter#531 (comment)

@encodes

This comment has been minimized.

Show comment
Hide comment
@encodes

encodes Apr 3, 2012

i agree, perhaps having any attributes that start data-* be passed to the modal. would be nice if multiple modals are required.

encodes commented Apr 3, 2012

i agree, perhaps having any attributes that start data-* be passed to the modal. would be nice if multiple modals are required.

@bjensen

This comment has been minimized.

Show comment
Hide comment
@bjensen

bjensen Apr 14, 2012

Good idea encodes

+1

bjensen commented Apr 14, 2012

Good idea encodes

+1

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Apr 16, 2012

Contributor

I havent figured out a good way to implement this actually.

I would like to use the naming convention of tabs with relatedTarget, and the easy part is easy:

doing $target.modal(option, this) and $.fn.modal = function (option, relatedTarget) {

But then Im stuck - need to somehow jinx in the relatedTarget into the modal instance data, or pass it in as argument on each call, which I dont like.

There is also the thing that the modal plugin should probably still work without the data-api which makes it more complicated.

Contributor

andriijas commented Apr 16, 2012

I havent figured out a good way to implement this actually.

I would like to use the naming convention of tabs with relatedTarget, and the easy part is easy:

doing $target.modal(option, this) and $.fn.modal = function (option, relatedTarget) {

But then Im stuck - need to somehow jinx in the relatedTarget into the modal instance data, or pass it in as argument on each call, which I dont like.

There is also the thing that the modal plugin should probably still work without the data-api which makes it more complicated.

@encodes

This comment has been minimized.

Show comment
Hide comment
@encodes

encodes Apr 16, 2012

i havn't wrote an implementation yet. but in my head, i feel that data-id='32' etc in the source code would be a good possible way of implementing it. would be easy and powerful, there would need to be other ways to set/get this data. but its a problem that can and im sure will be solved many ways

encodes commented Apr 16, 2012

i havn't wrote an implementation yet. but in my head, i feel that data-id='32' etc in the source code would be a good possible way of implementing it. would be easy and powerful, there would need to be other ways to set/get this data. but its a problem that can and im sure will be solved many ways

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Apr 16, 2012

Contributor

@encodes yeah problem is to pass the elem in somehow to the modal instance so that it can read the $(elem).data(), since the modal is instanciated once, and then reused the related target element from where the click originates needs to be passed in each time since the idea of this is that several links/sources could open the same modal.

(all this goes for collapsables as well)

Contributor

andriijas commented Apr 16, 2012

@encodes yeah problem is to pass the elem in somehow to the modal instance so that it can read the $(elem).data(), since the modal is instanciated once, and then reused the related target element from where the click originates needs to be passed in each time since the idea of this is that several links/sources could open the same modal.

(all this goes for collapsables as well)

@kxhawkins

This comment has been minimized.

Show comment
Hide comment
@kxhawkins

kxhawkins Apr 20, 2012

+1 for this. In my case, I want to use modals everywhere the user can input something. For instance, on my calendar, when the user clicks a day it opens an "Add event" modal with the date pre-populated.

+1 for this. In my case, I want to use modals everywhere the user can input something. For instance, on my calendar, when the user clicks a day it opens an "Add event" modal with the date pre-populated.

@palamedes

This comment has been minimized.

Show comment
Hide comment
@palamedes

palamedes May 6, 2012

+1

+1

@danger2k7

This comment has been minimized.

Show comment
Hide comment
@danger2k7

danger2k7 May 7, 2012

+1

+1

@bronson

This comment has been minimized.

Show comment
Hide comment
@bronson

bronson May 11, 2012

Big +1 for this. Right now I'm $('body').off('.data-api') and installing my own click handlers. Seems like a waste.

bronson commented May 11, 2012

Big +1 for this. Right now I'm $('body').off('.data-api') and installing my own click handlers. Seems like a waste.

@bronson

This comment has been minimized.

Show comment
Hide comment
@bronson

bronson May 11, 2012

Turns out this is actually pretty easy in 2.0.3. First, add your custom data as any number of data parameters.

<li><a data-person="Zim" data-toggle="modal" data-target="#person-modal"></a></li>
<li><a data-person="Dib" data-toggle="modal" data-target="#person-modal"></a></li>
...

Next, modify bootstrap-modal.js to update your modal's options every time a link is clicked. Otherwise, the modal always displays the data for the first person clicked.

diff --git a/app/assets/javascripts/bootstrap/bootstrap-modal.js b/app/assets/javascripts/bootstrap/bootstrap-modal.js
index 0465cb5..9228636 100644
--- a/app/assets/javascripts/bootstrap/bootstrap-modal.js
+++ b/app/assets/javascripts/bootstrap/bootstrap-modal.js
@@ -209,6 +209,9 @@
       var $this = $(this), href
         , $target = $($this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '')) //strip for ie7
         , option = $target.data('modal') ? 'toggle' : $.extend({}, $target.data(), $this.data())
+      if(option == 'toggle') {
+        $.extend($target.data('modal').options, $this.data())
+      }

       e.preventDefault()
       $target.modal(option)

Finally, use the show event to stuff your custom information into the modal.

    $('#person-modal').on('show', function (event) {
        name = $(this).data('modal').options.person
        $(this).find('.person').html(name)
    })

If @fat can make this patch presentable and commit it, this feature req can be closed.

bronson commented May 11, 2012

Turns out this is actually pretty easy in 2.0.3. First, add your custom data as any number of data parameters.

<li><a data-person="Zim" data-toggle="modal" data-target="#person-modal"></a></li>
<li><a data-person="Dib" data-toggle="modal" data-target="#person-modal"></a></li>
...

Next, modify bootstrap-modal.js to update your modal's options every time a link is clicked. Otherwise, the modal always displays the data for the first person clicked.

diff --git a/app/assets/javascripts/bootstrap/bootstrap-modal.js b/app/assets/javascripts/bootstrap/bootstrap-modal.js
index 0465cb5..9228636 100644
--- a/app/assets/javascripts/bootstrap/bootstrap-modal.js
+++ b/app/assets/javascripts/bootstrap/bootstrap-modal.js
@@ -209,6 +209,9 @@
       var $this = $(this), href
         , $target = $($this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '')) //strip for ie7
         , option = $target.data('modal') ? 'toggle' : $.extend({}, $target.data(), $this.data())
+      if(option == 'toggle') {
+        $.extend($target.data('modal').options, $this.data())
+      }

       e.preventDefault()
       $target.modal(option)

Finally, use the show event to stuff your custom information into the modal.

    $('#person-modal').on('show', function (event) {
        name = $(this).data('modal').options.person
        $(this).find('.person').html(name)
    })

If @fat can make this patch presentable and commit it, this feature req can be closed.

@d1rk

This comment has been minimized.

Show comment
Hide comment
@d1rk

d1rk May 11, 2012

yeah - would love to see that incorporated. It makes sense, breaks nothing and adds a lot of value to that component.

d1rk commented May 11, 2012

yeah - would love to see that incorporated. It makes sense, breaks nothing and adds a lot of value to that component.

@cipater

This comment has been minimized.

Show comment
Hide comment
@cipater

cipater May 17, 2012

+1

cipater commented May 17, 2012

+1

@Laurentvw

This comment has been minimized.

Show comment
Hide comment
@Laurentvw

Laurentvw May 21, 2012

+1!

+1!

@oferh

This comment has been minimized.

Show comment
Hide comment
@oferh

oferh May 23, 2012

+1

oferh commented May 23, 2012

+1

@scobal

This comment has been minimized.

Show comment
Hide comment
@scobal

scobal Jun 3, 2012

+1

scobal commented Jun 3, 2012

+1

@bronson

This comment has been minimized.

Show comment
Hide comment
@bronson

bronson Jun 6, 2012

Of course the constructor suffers the same problem. Here's a fix:

diff --git a/app/assets/javascripts/bootstrap/bootstrap-modal.js b/app/assets/javascripts/bootstrap/bootstrap-modal.js
index 0465cb5..a51813b 100644
--- a/app/assets/javascripts/bootstrap/bootstrap-modal.js
+++ b/app/assets/javascripts/bootstrap/bootstrap-modal.js
@@ -187,6 +187,7 @@
         , data = $this.data('modal')
         , options = $.extend({}, $.fn.modal.defaults, $this.data(), typeof option == 'object' && option)
       if (!data) $this.data('modal', (data = new Modal(this, options)))
+      else $.extend($this.data('modal').options, option)
       if (typeof option == 'string') data[option]()
       else if (options.show) data.show()
     })

Without this patch, modals only use the first options they were passed and ignore the ones passed to subsequent calls. With it, it uses the options as you'd expect.

Anyone willing to give this a code review? Hopefully it's time for a pull request.

bronson commented Jun 6, 2012

Of course the constructor suffers the same problem. Here's a fix:

diff --git a/app/assets/javascripts/bootstrap/bootstrap-modal.js b/app/assets/javascripts/bootstrap/bootstrap-modal.js
index 0465cb5..a51813b 100644
--- a/app/assets/javascripts/bootstrap/bootstrap-modal.js
+++ b/app/assets/javascripts/bootstrap/bootstrap-modal.js
@@ -187,6 +187,7 @@
         , data = $this.data('modal')
         , options = $.extend({}, $.fn.modal.defaults, $this.data(), typeof option == 'object' && option)
       if (!data) $this.data('modal', (data = new Modal(this, options)))
+      else $.extend($this.data('modal').options, option)
       if (typeof option == 'string') data[option]()
       else if (options.show) data.show()
     })

Without this patch, modals only use the first options they were passed and ignore the ones passed to subsequent calls. With it, it uses the options as you'd expect.

Anyone willing to give this a code review? Hopefully it's time for a pull request.

@andriijas

This comment has been minimized.

Show comment
Hide comment
@andriijas

andriijas Jun 7, 2012

Contributor

@bronson please make a pull request and refer to this issue as it has been closed for 7 month but people are still actively discussing and contributing to it.

Contributor

andriijas commented Jun 7, 2012

@bronson please make a pull request and refer to this issue as it has been closed for 7 month but people are still actively discussing and contributing to it.

@datacarl

This comment has been minimized.

Show comment
Hide comment
@datacarl

datacarl Jun 14, 2012

+1

+1

@kevinohashi

This comment has been minimized.

Show comment
Hide comment
@kevinohashi

kevinohashi Jun 18, 2012

@bronson I tested the code and it worked well. Thank you for that patch.

@bronson I tested the code and it worked well. Thank you for that patch.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 6, 2012

+2

ghost commented Jul 6, 2012

+2

@antonheryanto

This comment has been minimized.

Show comment
Hide comment
@antonheryanto

antonheryanto Jul 9, 2012

+2

+2

@tommyw

This comment has been minimized.

Show comment
Hide comment
@tommyw

tommyw Jul 11, 2012

+1, works well. Thanks

tommyw commented Jul 11, 2012

+1, works well. Thanks

@alexandrejobin

This comment has been minimized.

Show comment
Hide comment
@alexandrejobin

alexandrejobin Jul 19, 2012

+1, great work!
i whish it will be included in the next version!!

+1, great work!
i whish it will be included in the next version!!

@afanjul

This comment has been minimized.

Show comment
Hide comment
@afanjul

afanjul Jul 26, 2012

+3 for me!! this feature is a must for modal windows!!

afanjul commented Jul 26, 2012

+3 for me!! this feature is a must for modal windows!!

@SummerDew

This comment has been minimized.

Show comment
Hide comment
@SummerDew

SummerDew Jul 31, 2012

+9001

+9001

kynan added a commit to checklisthq/bootstrap that referenced this issue Aug 2, 2012

Allow passing data arguments to modal (fixes #531)
Implements @bronson's suggestion, all credit to him.

kynan added a commit to checklisthq/bootstrap that referenced this issue Aug 14, 2012

Allow passing data arguments to modal (fixes #531)
Implements @bronson's suggestion, all credit to him.
@asacarter

This comment has been minimized.

Show comment
Hide comment
@asacarter

asacarter Aug 16, 2012

How do I set a data value programtically without using "data-" in the HTML?

Using $('selector').data('user', value) seems to get overwritten by the last click from a data attribute.

I am trying to pass a data value from one modal to another after a successful ajax request but the data value gets overwritten. The problem is the data could also come from a HTML link instead of the first modal.

$('#modal2').data('user_id', 5) // sets user_id to 5

$('#modal2').on('show', function (event) {
$('#modal2').data('user_id', $('#modal2').data('modal').options.user_id ); // sets user_id to something else
}

Link on SO to better explain it:

http://stackoverflow.com/questions/11992128/twitter-bootstrap-modal-issue-531

How do I set a data value programtically without using "data-" in the HTML?

Using $('selector').data('user', value) seems to get overwritten by the last click from a data attribute.

I am trying to pass a data value from one modal to another after a successful ajax request but the data value gets overwritten. The problem is the data could also come from a HTML link instead of the first modal.

$('#modal2').data('user_id', 5) // sets user_id to 5

$('#modal2').on('show', function (event) {
$('#modal2').data('user_id', $('#modal2').data('modal').options.user_id ); // sets user_id to something else
}

Link on SO to better explain it:

http://stackoverflow.com/questions/11992128/twitter-bootstrap-modal-issue-531

@bronson

This comment has been minimized.

Show comment
Hide comment
@bronson

bronson Aug 27, 2012

@asacarter I do this: $('#my-modal').modal({person:'Gaz'}). If the modal is hidden, that will set modal.options.person and then call your show handler. (I think it toggles so, if it's showing, it gets hidden... fuzzy memory tho)

You need to apply the one-line constructor patch of course.

bronson commented Aug 27, 2012

@asacarter I do this: $('#my-modal').modal({person:'Gaz'}). If the modal is hidden, that will set modal.options.person and then call your show handler. (I think it toggles so, if it's showing, it gets hidden... fuzzy memory tho)

You need to apply the one-line constructor patch of course.

@FacioRatio

This comment has been minimized.

Show comment
Hide comment
@FacioRatio

FacioRatio Sep 7, 2012

I can't get the 'show' event callback to fire. (My modal is showing.) Do I need to put that code anywhere in particular?
Edit: nevermind, copy-paste error. Works great!

I can't get the 'show' event callback to fire. (My modal is showing.) Do I need to put that code anywhere in particular?
Edit: nevermind, copy-paste error. Works great!

@akshaysmurthy

This comment has been minimized.

Show comment
Hide comment
@akshaysmurthy

akshaysmurthy Sep 10, 2012

+1

kynan added a commit to checklisthq/bootstrap that referenced this issue Sep 24, 2012

Allow passing data arguments to modal (fixes #531)
Implements @bronson's suggestion, all credit to him.

kynan added a commit to checklisthq/bootstrap that referenced this issue Dec 13, 2012

Allow passing data arguments to modal (fixes #531)
Implements @bronson's suggestion, all credit to him.
@niftylettuce

This comment has been minimized.

Show comment
Hide comment
@niftylettuce

niftylettuce Apr 7, 2013

👍

👍

@leefaus

This comment has been minimized.

Show comment
Hide comment
@leefaus

leefaus May 29, 2013

FYI: Playing around with 3.0.
2.3.1
name = $(this).data('modal').options.person
3.0.0
name = $(this).data('bs.modal').options.person

Can anyone confirm this?

I also think the patch has not been applied because I am only seeing the first element clicked.

leefaus commented May 29, 2013

FYI: Playing around with 3.0.
2.3.1
name = $(this).data('modal').options.person
3.0.0
name = $(this).data('bs.modal').options.person

Can anyone confirm this?

I also think the patch has not been applied because I am only seeing the first element clicked.

@asgeo1

This comment has been minimized.

Show comment
Hide comment
@asgeo1

asgeo1 Jun 11, 2013

Agree with @leefaus, only data-* values from the first element clicked are available as options. Tested in v2.3.1

Bit disappointing... I thought some of the previous comments alluded to this having been fixed :(

asgeo1 commented Jun 11, 2013

Agree with @leefaus, only data-* values from the first element clicked are available as options. Tested in v2.3.1

Bit disappointing... I thought some of the previous comments alluded to this having been fixed :(

@cvrebert

This comment has been minimized.

Show comment
Hide comment
@cvrebert

cvrebert Jun 11, 2013

Member

#6825 is the most recent word on this. @fat is still "noodling".

Member

cvrebert commented Jun 11, 2013

#6825 is the most recent word on this. @fat is still "noodling".

@abrudtkuhl

This comment has been minimized.

Show comment
Hide comment
@abrudtkuhl

abrudtkuhl Jun 19, 2013

+1 Any progress on this?

Confirmed: Only the first element clicked passes data-* in v2.3.1

+1 Any progress on this?

Confirmed: Only the first element clicked passes data-* in v2.3.1

@alvincrespo

This comment has been minimized.

Show comment
Hide comment
@alvincrespo

alvincrespo Jun 21, 2013

+1

+1

@jtal

This comment has been minimized.

Show comment
Hide comment
@jtal

jtal Jun 26, 2013

Is there a fund somewhere that I can contribute to fix this > 2 year old bug? :>

jtal commented Jun 26, 2013

Is there a fund somewhere that I can contribute to fix this > 2 year old bug? :>

@simonwh

This comment has been minimized.

Show comment
Hide comment
@simonwh

simonwh Jul 4, 2013

+1

simonwh commented Jul 4, 2013

+1

@danielsantiago

This comment has been minimized.

Show comment
Hide comment
@danielsantiago

danielsantiago Jul 11, 2013

+1

@jgc94131

This comment has been minimized.

Show comment
Hide comment
@jgc94131

jgc94131 Jul 22, 2013

+1
I have to say this is a pretty obvious bug that needs to be fixed, and it greatly increases the utility of the modal widget and the ease by which it can be used. Of course the other option is to implement relatedTarget. Both have been suggested and progress is stalled.

+1
I have to say this is a pretty obvious bug that needs to be fixed, and it greatly increases the utility of the modal widget and the ease by which it can be used. Of course the other option is to implement relatedTarget. Both have been suggested and progress is stalled.

@mpaf

This comment has been minimized.

Show comment
Hide comment
@mpaf

mpaf Oct 29, 2013

+1

mpaf commented Oct 29, 2013

+1

@cvrebert

This comment has been minimized.

Show comment
Hide comment
@cvrebert

cvrebert Oct 29, 2013

Member

Folks, this is addressed in v3.0.0+ thanks to event.relatedTarget getting implemented in dbed9da.

Member

cvrebert commented Oct 29, 2013

Folks, this is addressed in v3.0.0+ thanks to event.relatedTarget getting implemented in dbed9da.

@cvrebert cvrebert locked and limited conversation to collaborators Jun 14, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.