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

Removed 'return false' in favor of preventDefault. #24

Merged
merged 1 commit into from May 18, 2012

Conversation

Projects
None yet
7 participants
@marcneuwirth
Contributor

marcneuwirth commented Apr 30, 2012

Using 'return false' makes it very difficult to add additional event handlers to the add button because it prevent the events from bubbling up; preventDefault appears to be the desired behavior.

Also, other various js fixes like === instead of ==, missing parseInt radix, and missing semi colons

https://code.djangoproject.com/ticket/18241

@adrianholovaty

This comment has been minimized.

Member

adrianholovaty commented May 1, 2012

Hey, this is looking good. I have one question, though -- should the preventDefault calls perhaps be at the start of the event handler instead of the end? That way, if there's a runtime error in the handler, the link won't get followed (even though its href is "#").

See the Stack Overflow answer by Jeff Poulton here for a better explanation: http://stackoverflow.com/questions/1357118/event-preventdefault-vs-return-false

Worth changing?

@marcneuwirth

This comment has been minimized.

Contributor

marcneuwirth commented May 1, 2012

Good idea

@eduardocereto

This comment has been minimized.

Contributor

eduardocereto commented May 9, 2012

Note that internet explorer 8- doesn't support preventDefault. So you probably want to do this instead:

if (e.preventDefault) { 
    e.preventDefault(); 
} else {
    e.returnValue = false;
}
@mlavin

This comment has been minimized.

Contributor

mlavin commented May 9, 2012

IE doesn't natively support e.preventDefault but this is will work inside a jQuery event handler because jQuery.Event defines preventDefault https://github.com/jquery/jquery/blob/1.4.2/src/event.js#L566

@alex

This comment has been minimized.

Member

alex commented May 10, 2012

Looks good to me, Adrian, are you good with this?

@adrianholovaty

This comment has been minimized.

Member

adrianholovaty commented May 11, 2012

This looks good -- thanks, guys. I'd like us to start squashing our pull requests, though, so that I end up making only one commit to the Django master repository. @marcneuwirth -- can you squash your commits into one?

adrianholovaty added a commit that referenced this pull request May 18, 2012

Merge pull request #24 from marcneuwirth/master
Removed 'return false' in favor of preventDefault in admin JS.

@adrianholovaty adrianholovaty merged commit 04785d2 into django:master May 18, 2012

@adrianholovaty

This comment has been minimized.

Member

adrianholovaty commented May 18, 2012

Thank you!

@jezdez

This comment has been minimized.

Contributor

jezdez commented May 19, 2012

The minified version needs to be updated, too.

@jphalip

This comment has been minimized.

Member

jphalip commented Jun 16, 2012

The minified version was updated in fadcc6d.

@jezdez

This comment has been minimized.

Contributor

jezdez commented Jun 17, 2012

Odd, pretty sure I updated the files, at the time, too.

nanuxbe pushed a commit to nanuxbe/django that referenced this pull request Jul 2, 2016

Merge pull request django#24 from ramiro/08b96606e932a5ece464425d0c73…
…c3154b5bef3f

Clean docs destdir before Spginx renders them
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment