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

Fix for not properly closed modal #23

Closed
wants to merge 1 commit into from
Closed

Fix for not properly closed modal #23

wants to merge 1 commit into from

Conversation

jstri
Copy link

@jstri jstri commented Nov 26, 2014

After modal was removed from DOM, there was still possibility that "modal-open" class was left on body element. It happened only in case if modal buttons didn't have data-dismiss="modal" attribute. This fix triggers normal bootstrap hiding process and removes element from DOM afterwards.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.7%) when pulling 2bad891 on siready:master into 716163b on dwmkerr:master.

@jstri
Copy link
Author

jstri commented Nov 26, 2014

Tests are failing obviously. Could you please verify and check the problem? Then we can tackle tests.

@dwmkerr
Copy link
Owner

dwmkerr commented Nov 27, 2014

Hi @siready the problem with this code is that it assumes that every modal is a bootstrap modal, and the library is designed to be agnostic as to what types of modal are being used. The implication is that any bootstrap specific functionality should be implemented by consumers of the library rather than the library itself.

The code however is useful, so it might be worth adding to the documentation as a tip for bootstrap users, what do you think?

@jstri jstri closed this Mar 1, 2015
@jstri
Copy link
Author

jstri commented Mar 1, 2015

You're right! That approach was wrong, but at the time I wasn't aware it's supposed to be vendor agnostic.

I eventually settled with closing the dialog with the following code in modal controller:

$element.modal('hide');
$element.one('hidden.bs.modal', function(){
    close();
});

@dwmkerr
Copy link
Owner

dwmkerr commented Mar 2, 2015

I like it, I'll write it up!

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

3 participants