Add option to Modal to force refresh the content.

[josephdpurcell]

Add option to Modal to force refresh the content. This option allows modals to be dynamic and multipurpose. For example, you can use the same modal div for the following two links that request different content:

<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 can be tested at http://jsfiddle.net/sCsB9/.

(This pull request deprecates pull request #6793.)

[josephdpurcell]

In short, here is a JSFiddle that works http://jsfiddle.net/sCsB9/ and if you want to know why the previous pull request didn't work, keep reading.

I was confounded by the issues observed in the jsfiddle @mediafreakch made in his comment (#6793 (comment)) on the previous pull request I made. And, in light of so much having changed in 3.0.0-wip over the last three months I started from scratch.

Bootstrap modals were designed to be static and single-purpose. Myself and others have interest in making them dynamic and multi-purpose. This pull request hopes to achieve that by adding a "refresh" option and method to run jQuery.load() and pull fresh content from the remote URL. I considered several ways of accomplishing this but came to the same conclusion as last time: make an option to force update the content.

The result is this pull request. I fiddled with @mediafreakch's jsfiddle and made this one: http://jsfiddle.net/sCsB9/. Per a few tests, it behaves as expected.

To see the fix for the modal code as it was 3 months ago compared with now I created the following gist: https://gist.github.com/josephdpurcell/5612293. The flaw in my previous request was the data api click handler did not update the modal options. So, it would keep the same remote and refresh options from when it was instantiated. Initializing the modal via JavaScript, however, did not experience this issue because the new options were passed to the refresh method and updated. (As an aside regarding the duplicate requests @mediafreakch saw (#6793 (comment)) in Firefox, I saw the same with the previous code, but no occurrence of this with the new code.)

One final note: I found no way to test if my updates to the javascript.html documentation are correct. However, I checked and double checked and I believe the syntax and markup are correct.

[mediafreakch]

Thanks a lot @josephdpurcell for your effort! It does what it should and works as expected now. I think it really adds value to the default bootstrap modal function and many people would appreciate seeing this beeing included in 3.0.0!

However in order to make it as easy as possible for the maintainers to integrate it, someone should provide an automated test for it.

[jokklan]

There is sometimes a little delay on the rendering of the new html. Could the rendering be in a callback for jQuery.load()?

[josephdpurcell]

@mediafreakch, my apologies, that is a large oversight on my part. I had been reading the contrib docs from master, not wip-3.0.0! Which, also explains how to build the HTML docs. That's about as embarrassing as my former pull request being buggy code! I will plan to complete this within the next 24-48 hours while it's still fresh.

@jokklan, thank you for the suggestion. I agree that the rendering should be smooth. The reason I avoided moving the rendering code to the callback for jQuery.load() was because of the if (this.options.remote) statement, which would imply that if the rendering was within the load callback it will not be run if there is no remote URL. The obvious solution would be to put the rendering within the jQuery.load() callback and the else case of the if statement, but then we have duplicate code.

That said, I think living with that duplicate code isn't a problem, because (a) it's two lines of code and (b) all the rendering code does is say "go on to the show() method if our modal isn't shown and the show option is set to truthy" (see lines 98 and 100 of modal.js). Theoretically it is highly unlikely that code will change; the only cases I can think of off hand would be if the method or option names changed.

In conclusion, I will plan to move the rendering code inside the jQuery.load() callback and duplicate that code inside the else case of the if (this.options.remote) statement. This solution will not make the rendering smooth, but will enable the user to add candy while jQuery.load() is running by listening in on the refresh.bs.modal and refreshed.bs.modal events. However, if anyone has any objections or concerns not listed here I will reconsider.

[fat]

this doesn't close now if refresh is true

[josephdpurcell]

Okay, sorry for the delay. I had to carve out additional time to setup Jekyll and make sure the documentation builds correctly, now that I know how. Thank you for being patient.

First of all, I did not realize that force updating would update this pull request. I intended to make a completely new request, but it would seem that is not needed now. Please tell me otherwise, if so.

Second, @mediafreakch, I tried matching as best I could what unit tests were already there when I wrote tests for refresh. The following four tests I created are straightforward: (1) test if model is inserted into DOM on refresh, (2) check for refresh event trigger, (3) should not update content when remote is not given, and (4) should not fire refreshed event when default is prevented. The one new type of test I added was for checking if the modal content is updated, which involved modifying the server.js file. I hope I am not over stepping any boundaries here, but I could not think of another practical way to test this. So, I added a request handler with only one recognized path, /model/example1.html.

Third, @fat, I took your note into account and realized that the toggle method doesn't even need changing to begin with, if I understand the intent of that method correctly. I believe the method is there to show and hide the modal. As such, I don't believe it makes sense to refresh the modal content in that method. I believe that method should be preserved for merely showing and hiding the modal and only pulling remote content when initializing the modal for the first time.

Lastly, I have updated the JSFiddle with more description and buttons: http://jsfiddle.net/sCsB9/4/.

[mediafreakch]

@josephdpurcell I see duplicated requests again on first load for all links that have set "refresh" to true (Firefox 20.0.1). Is it because jQuery .load() is called twice if a modal is initialized the first time: once on line 32 and then again in your refresh method on line 93?

Furthermore, on line 255, shouldn't it be $target.data('bs.modal') . It wasn't introduced by your commit, but maybe you can fix it?

Regarding documentation: I wonder if it is necessary to tell the user that if he uses the same modal to display different contents, that he needs to set refresh="true" for all links that point to that modal? If so, you could write something like:
If using the same modal to load different data-targets, make sure you set refresh="true" for all links that point to that modal in order to avoid unwanted sideeffects.

Looking forward to the feedback from @fat or @mdo whether this will be implemented in BS 3.0.0. Thanks again for your effort @josephdpurcell

[fat]

Did you consider that in bootstrap 3 you could just add this to your js:

$(document.body).on('hidden.bs.modal', function () {
  $('#myModal').removeData('bs.modal')
});

This would cause all modal instances to dispose on close and allow you to "refresh" content from different remote sources.

I guess I'm not completely convinced we need to build this into the plugin

[josephdpurcell]

@mediafreakch, I'm seeing the duplicate requests now as well. I agree with your other comments as well.

However, @fat, your suggestion works well: http://jsfiddle.net/sCsB9/9/, and it requires no modification to Bootstrap. (Note that the event is "hidden.bs.modal".)

Also, it works on 2.3.0: http://jsfiddle.net/EbLuk/.

As @fat shows, this "refresh" functionality can be achieved outside of Bootstrap modal code. However, I don't think this is an obvious solution to someone who is not familiar with the Bootstrap modal code. (Though, to me it was so obvious it seemed too good to be true.) I think making an obvious way to achieve this "refresh" ability would add enough value to Bootstrap to (a) add @fat's suggestion to the documentation as the recommended way to achieve this "refresh" ability, (b) scrap this pull request and make a skinnier version using @fat's suggestion in the "hide" method, or (c) continue with this pull request.

I'm going to follow (c) and make a JSFiddle for (b) just to see what it will be like, unless in the mean time there is consensus that this "refresh" functionality should be completely left out.

[mediafreakch]

The advantage of (c) is that one'll have a method "refresh" to refresh the content from within the modal, which would be more complicated otherwise, I think.

[mediafreakch]

To be clear: Option c (this pull request) makes it possible to easily build stacked modal dialogs (change modal content from within the modal itself) by just using the same technique used to trigger the first modal. Which I think is a big win for that small piece of code!

[fat]

hm @josephdpurcell this is rad pr (w tests ❤️) but i think i'd prefer option A. Trying to keep the options super lean for bootstrap and the alternative is literally so simple.

@mediafreakch you can call $('#myModal').removeData('bs.modal') at anytime which is the same a refresh method.