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 option to Modal to force refresh the content. #6793

Closed
wants to merge 1 commit into from

Conversation

josephdpurcell
Copy link

Add option to Modal to force refresh the content. This option is useful for modals that show remote content that may change, and for using the same modal div for different modals. For example, the following HTML will work as expected:

<a href="/api/pages/user/profile" data-toggle="modal" data-target="#myModal" data-refresh="true">My Profile</a>
<a href="/api/pages/user/settings" data-toggle="modal" data-target="#myModal" data-refresh="true">Account Settings</a>

This feature has only been tested in Chrome Version 24.0.1312.57.

@fat
Copy link
Member

fat commented Feb 6, 2013

don't think we want to add this to core – thanks tho!

@jonlives
Copy link

Out of curiosity, why didn't this get merged? It strikes me as an extremely useful feature.

@josephdpurcell
Copy link
Author

I am curious as well. Chris Rebert's comment on #6865 was a rather comprehensive list of references to issues and pull requests for the same issue this code addresses: being able use the same modal DOM for injecting different remote content.

Given that this request was rejected but not for lack of completeness, I am guessing the reason is the same as what Corin Lawson said about #4468, "the remote ability of the modal component is not intended to be in the way that you're suggesting, it is there to reduce load time (lazy loading FTW)." Thus, Bootstrap modals only serve the case for single-purpose modals and the remote ability simply helps load time if the modal is large.

Even so, I hope Bootstrap will re-consider expanding the vision of what modals can do and it would seem the Bootstrap community would agree.

@josephdpurcell
Copy link
Author

As an additional note, I would like to clarify the purpose I intended: I would like to see Bootstrap modals address the case for a multi-purpose modal (one modal for many purposes) and the remote ability be the mechanism that allows more than one purpose for the modal. The example I gave at the beginning epitomizes this idea: a link to open the user's profile and a link to open the user's account settings that use the same modal.

@mediafreakch
Copy link
Contributor

Would be nice to have! Unfortunately the code in the commit from @josephdpurcell doesn't work. Content doesn't refresh because URL to be loaded doesn't change. Also on first click, content gets loaded twice. See jsFiddle: http://jsfiddle.net/YzxcS/ (Use your Browser's network tab to see the requests - I used just fake-urls)

@mediafreakch
Copy link
Contributor

And the reason why loading different content into the same modal doesn't work can be found on line 255 of bootstrap-modal.js:

option = $target.data('modal') ? 'toggle' : $.extend({ remote:!/#/.test(href) && href }, $target.data(), $this.data())

This already takes the cached data-attributes if modal had been clicked previously... Can you fix that somehow depending on the data-refresh option?

@josephdpurcell
Copy link
Author

Huh. Interesting. I see nothing wrong with your jsfiddle, @mediafreakch, but I distinctly recall it working as you intended, and certainly not making duplicate requests for the remote! I'll take a look at it this weekend and post my findings.

@mediafreakch
Copy link
Contributor

@josephdpurcell thx for the quick reply! So I had a look again. The duplicate requests occur for e.g. Firefox (21.0), but not Chrome. Regarding the intended behaviour: On the screenshot from firebug you can see that the url requested for button 1 is the same as for button 2. Would be great if you could have a look at this!
bildschirmfoto 2013-05-17 um 10 36 56 pm

@josephdpurcell
Copy link
Author

In continuation of our conversation, @mediafreakch, I want to add an update here.

I hope my actions make sense, please tell me otherwise. I have submitted a new pull request #7935 that is for the latest 3.0.0-wip. That request should deprecate this one.

As such, I'm going to add further comments to that request.

@ajeliuc
Copy link

ajeliuc commented Jun 26, 2013

We need this feature!

@ajeliuc
Copy link

ajeliuc commented Jun 26, 2013

 $.fn.modal = function (option) {
    return this.each(function () {
      var $this = $(this)
        , data = $this.data('modal')
        , options = $.extend({}, $.fn.modal.defaults, $this.data(), typeof option == 'object' && option)
      if (!data || options.refresh) $this.data('modal', (data = new Modal(this, options)))
      if (typeof option == 'string') data[option]()
      else if (options.show) data.show()
    })
  }

@avanwart
Copy link

Better not to have this feature out of the box. If @fat implemented every merge request that has a few +1s, Bootstrap would cease to be the light-weight awesomeness that it is.

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.

None yet

6 participants