Skip to content

Fixes: #7327 - Dialog box size and close animation bugs #300

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

Closed
wants to merge 5 commits into from

Conversation

davidmurdoch
Copy link

This actually only fixes the close animation bug. I couldn't reproduce the dialog size issue stated in the ticket. I'm thinking the reporter may have been thrown off by the padding and border added by the ui theme.

The fix is pretty simply though: don't close a dialog that has already been closed.

Ticket 7327: http://bugs.jqueryui.com/ticket/7327

David Murdoch added 3 commits May 17, 2011 17:03
…eported in the ticket doesn't' seem to be a problem (maybe the padding and border from the ui theme was throwing the ticket-reporter off?)
@davidmurdoch
Copy link
Author

Let me know that merge with "upstream master" there is going to mess things up. I'm not really sure what I did to put that there...but if it is a problem (I don't think it will be because it is up to date with you) let me know and I'll try to fix it.

@gnarf
Copy link
Member

gnarf commented May 24, 2011

I actually can't seem to reproduce the bug in the first place using your test case... But either way, it kind of makes sense to not fire close code if !this._isOpen - I say its pullable (with a squash) @scottgonzalez

if ( self.options.hide ) {
self.uiDialog.hide( self.options.hide, function() {

if( self._isOpen ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spacing: if ( self._isOpen ) {

Please invert this so that it becomes:

if ( !self._isOpen ) {
    return self;
}

@scottgonzalez
Copy link
Member

Thanks, landed in c7eae7b. I moved the check to the very top of close().

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

Successfully merging this pull request may close these issues.

3 participants