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

Compatibility with Bootstrap 3 #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rchampourlier
Copy link

This is a basic/hacky compatibility update. I essentially made the diff between bootstrap-modal 2.1 JS files and Bootstrap 3's modal.js, then merged them.

Some features of Bootstrap modal may be broken, but it seems to work for the most at this point.

I'll keep this branch updated as soon as I identify something missing. I'm sorry I could not completely review the code to ensure it was rightfully done, but I hope it may help with starting the update.

This is a basic compatibility update. I essentially made the diff between bootstrap-modal 2.1 JS files and Bootstrap 3's modal.js, merged them.

Some features of Bootstrap modal may be broken, but it seems to work for the most at this point.
@vahid-sohrabloo
Copy link

+1

@Crempa
Copy link

Crempa commented Sep 2, 2013

not working closing modals for me, anyway +1 for bootstrap 3 compatibility, so thanks for this patch

@brunomac
Copy link

brunomac commented Sep 2, 2013

+1

Bruno Machado

On Mon, Sep 2, 2013 at 2:35 PM, Crempa notifications@github.com wrote:

not working closing modals for me, anyway +1 for bootstrap 3
compatibility, so thanks for this patch


Reply to this email directly or view it on GitHubhttps://github.com//pull/164#issuecomment-23660602
.

@jschr
Copy link
Owner

jschr commented Sep 2, 2013

Hey thanks for the PR. I ended up accomplishing this with just a simple css patch file. I didn't want to break backwards compatibility at this point so for now you'll need to include an extra css file for BS3.

Hope this works for most folks for the time being.

@rchampourlier
Copy link
Author

Hi @jschr,

When you say you "ended up accomplishing with just a simple css patch file", you mean the closing modals issue on my PR or the whole compatibility thing?

If this is for the whole Bootstrap 3 compatibility, I'm curious! Did you share it somewhere?

Thanks & cheers!

@jschr
Copy link
Owner

jschr commented Sep 3, 2013

@rchampourlier My goal for addressing BS3 was to not break backwards compatibility with BS2 as this plugin was originally created to address the problems with BS2's default modal.

Since lots of folks are still on BS2 and probably will be for quite some time I wanted to leave this plugin written primarily for BS2 but offer an upgrade patch. I believe BS3 default modals address a lot of the issues that this plugin was created to fix.

That being said I'll leave this PR open for the meantime as I'm sure there are some people would find this useful as well.

Thanks

@rchampourlier
Copy link
Author

Oh ok, I haven't even tried to play with BS 3 modals without your plugin. I should try this now indeed, thanks for the details!

@kliakos
Copy link

kliakos commented Sep 8, 2013

I have tested bs3 and the ajax modal has some issues. After it closes, the page-overflow class stays on the body, not letting any click on the page.

Please use the actual bs3 files from the cdn in your bs3.html example to ensure that everything works with bs3.

@urzur
Copy link

urzur commented Sep 8, 2013

@liago86 I am also hitting this issue !

@jschr
Copy link
Owner

jschr commented Sep 8, 2013

@liago86, @urzur Can you guys be more specific or provide an example of the problem? Unless I'm missing something I'm not seeing any problems with the Ajax example on the demo page in chrome.

@liago86 I'm using the same files as www.getbootstrap.com last I checked, if you can provide a link to the cdn that would be appreciated.

@kliakos
Copy link

kliakos commented Sep 8, 2013

@jschr The js used in the bs3 demo page example is the http://getbootstrap.com/2.3.2/assets/js/bootstrap.js.

The official bs3 js from the cdn is the //netdna.bootstrapcdn.com/bootstrap/3.0.0/js/bootstrap.min.js.

Replace the one used now with the one i mention, and you will see the problem at the AJAX (via jQuery.load) example. The problem is that when the modal is closed, the html body still has a "page-overflow" class.

I am not sure how i can provide more details.

@kliakos
Copy link

kliakos commented Sep 8, 2013

@jschr I just re-tested the case and i really can't reproduce the same bug. I am not sure what is going on.

I will come back with more details if i will be able to reproduce the bug.

@kliakos
Copy link

kliakos commented Sep 9, 2013

@jschr Cool, i saw you updated your demo page with the bs3 cdn resources.

Now, please check http://jschr.github.io/bootstrap-modal/bs3.html with Firefox 23.0.1, and you will see the bug i have mentioned. With Chrome or IE10 it's fine.

@kosekk
Copy link

kosekk commented Sep 9, 2013

Boostrap 3 stackable-modals demo doesnt work. (firefox 23)

@kliakos
Copy link

kliakos commented Sep 9, 2013

@kosekk The bug you are mentioning is being caused by the same reason with mine. The page-overflow class remains on the html body after the modal is closed.

@kosekk
Copy link

kosekk commented Sep 10, 2013

I couldn't wait for update so I made some changes in bootstrap-modalmanager.css - it is just a line, but it works for me and removes backdrop layer.

May be helpfull (line in file: 286) :
"
} else if (!modal.isShown && modal.$backdrop) {
modal.$backdrop.removeClass('in');

            this.backdropCount -= 1;

            var that = this;

            $.support.transition && modal.$element.hasClass('fade')?
                modal.$backdrop.one($.support.transition.end, function () { that.removeBackdrop(modal) }) :
                that.removeBackdrop(modal);
        /* line below pathes backdrop layer removing */     
            $('<div class="modal-scrollable">').remove();

"

@kosekk
Copy link

kosekk commented Sep 10, 2013

this works much better:

            /* 2 lines below pathes backdrop layer removing */
            $('.modal-scrollable').remove();
            $('.modal-backdrop').remove();

@urzur
Copy link

urzur commented Sep 11, 2013

kosekk, thanks for posting this. Works great and corrects the darker background on every call to the modal. Has anyone figured out how to bring up click events ? After the modal closes, once my page is done, my click events disappear :/

@trixter78
Copy link

If the DIV with class 'modal-scrollable' is removed the modal does not open again. When I just hide it, it works.

/* 2 lines below pathes backdrop layer removing */
$('.modal-scrollable').hide();
$('.modal-backdrop').remove();

@larbear
Copy link

larbear commented Sep 11, 2013

+1 for click events issue. Using Firefox 23 Windows, the bs3 demo page doesn't work anymore after closing a single modal. Works OK in Chrome.

@jschr
Copy link
Owner

jschr commented Sep 11, 2013

I believe I have a fix for this issue you guys are having: 3731fd5

Let me know if that works.

Thanks

@kliakos
Copy link

kliakos commented Sep 11, 2013

Nice, it works fine now on Firefox. Thanks a lot @jschr !

@richterfritz
Copy link

When using Bootbox.js together with this patch, it does not work. Bootstrap3 introduced the two wrappers ".modal-dialog" and ".modal-content" which, when used (according to the BS3 specs) are not working. The result is a double modal frame/border.

@ruifigueira
Copy link

I'm also experiencing the double modal frame/border issue reported by @richterfritz with BS3. For the time being, I'm removing the .modal-dialog div as a quick fix.

@ahazelwood
Copy link

I'm also having the same issue as @richterfritz with BS3.
image

@ahazelwood
Copy link

If I add the following to the bootstrap-modal-bs3patch.css, it fixes the double border for me (and also makes it look nicer in my opinion because of less padding for the dialog).

.modal-dialog {
width: auto;
padding: 10px 0;
}
.modal-footer {
padding: 10px;
}
.modal-content {
padding: 10px;
border: none;
box-shadow: none;
padding: 0;
margin: 0;
}

image
image

@ahazelwood
Copy link

One last thing. I also changed the width of the modal definition in bootstrap-modal-bs3patch.css to be 600px since that matches up to bs3.

/*!

  • Bootstrap Modal
    *
  • Copyright Jordan Schroter
  • Licensed under the Apache License v2.0
  • http://www.apache.org/licenses/LICENSE-2.0
    *
  • Boostrap 3 patch for for bootstrap-modal. Include BEFORE bootstrap-modal.css!
    */

body.modal-open,
.modal-open .navbar-fixed-top,
.modal-open .navbar-fixed-bottom {
margin-right: 0;
}

.modal {
left: 50%;
bottom: auto;
right: auto;
padding: 0;
width: 600px;
margin-left: -300px;
background-color: #ffffff;
border: 1px solid #999999;
border: 1px solid rgba(0, 0, 0, 0.2);
border-radius: 6px;
-webkit-box-shadow: 0 3px 9px rgba(0, 0, 0, 0.5);
box-shadow: 0 3px 9px rgba(0, 0, 0, 0.5);
background-clip: padding-box;
}
.modal-dialog {
width: auto;
padding: 10px 0;
}
.modal-footer {
padding: 10px;
}
.modal-content {
padding: 10px;
border: none;
box-shadow: none;
padding: 0;
margin: 0;
}
.modal.container {
max-width: none;
}

@Calabonga
Copy link

Template for modal windows are different! Modal window does not close by clicking dismiss button

@ghost
Copy link

ghost commented Oct 2, 2013

The 600px modal seems to create other issues. I was having an issue with multiple modals on BS3, and it was because my second modal was defaulting to smaller than 600px, but the modal window wouldn't allow it. See attached image
image
I corrected by adding $('#myModal').modal().children('.modal-dialog').width('auto') when calling the modal .Not a clean fix, but it works for today.

@heaven
Copy link

heaven commented Oct 12, 2013

That's not a patch, it overrides the entire library (what it shouldn't). I was looking over the changes and it is not possible to say what's namely changed to fix the compatibility. Push request should only contain related changes.

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

Successfully merging this pull request may close these issues.

None yet