[Cookbook/Form] Fixed jQuery bug #2158
Conversation
| $addTagLink.on('click', function(e) { | ||
| // prevent the link from creating a "#" on the URL | ||
| e.preventDefault(); | ||
|
|
||
| // add a new tag form (see next code block) | ||
| addTagForm(collectionHolder, $newLinkLi); | ||
|
|
||
| // `e.preventDefault()` for old browsers | ||
| return false; |
There was a problem hiding this comment.
I don't like this - this stops event propagation, which is a bad practice. Plus, this is a jQuery event, not a normal DOM event, so my impression is that jQuery would make sure that it's compatible with all browsers. Let me know what you think :).
There was a problem hiding this comment.
@weaverryan thank you for mentioning this. I haven't worked with jQuery for a long time, so I didn't know how jQuery would handle this.
I have done some research and it looks like return false in a jQuery event will cause executing e.preventDefault() and e.stopPropagation() (where e.preventDefault() is executed in a raw JS event).
On the other hand, e.preventDefault() in a jQuery object will execute e.returnValue = false if preventDefault does not exists (which is the method to prevent defaults in old IE browsers).
I'm going to remove this as it does things we don't want it to do. But we should keep this in mind, jQuery 2.0 will not support old IE browsers, which means that the e.preventDefault() method will not execute e.returnValue = false if preventDefault does not exists, which means that we need to do it ourselves.
|
@wouterj Really nice work looking into this - I just have one comment, and then I'll merge and do the correct steps when merging up. Thanks! |
| var newForm = prototype.replace(/\$\$name\$\$/g, index); | ||
|
|
||
| // increase the index with one for the next item | ||
| collectionHolder.attr('index', index + 1); |
[Cookbook/Form] Fixed jQuery bug
|
Thanks again @wouterj! I've merged into 2.1 and changed to |
Description
This PR fixes the old bug while adding new items. If you press the 'add a tag' button 4 times you get the indexes
0, 1, 2, 3. If you remove 1 and 2, you have the indexes0, 3. If you press the 'add a tag' button twice after this, you expect to get0, 3, 4, 5, but you got0, 3, 2, 3.This PR saves the index in a
data-indexattribute on the$collectionHolder.Live examples
Before: http://jsfiddle.net/WouterJ/847Kf/
After: http://jsfiddle.net/WouterJ/847Kf/1/
Side fixes
This PR also improves the jQuery code by adding support of
e.preventDefault()for old browsers and by using thedatamethod to getdata-*attributes.At least, this PR fixes a Sphinx syntax bug (
.. index:instead of.. index::)Merge instructions
Because the placeholder changed from
$$name$$to__name__, line 408 should be changed when merging this to2.1andmaster.