Traversing: Let .not(arraylike) pass non-element nodes #3261
Conversation
|
@dmethvin, thanks for your PR! By analyzing the annotation information on this pull request, we identified @timmywil, @markelog and @gibson042 to be potential reviewers |
test/unit/traversing.js
Outdated
|
|
||
| assert.deepEqual( mixedContents.not( mixedContents.get() ).get(), [], "not everything" ); | ||
| assert.deepEqual( mixedContents.not( childElements ).length, mixedLength - 1, "not childElements" ); | ||
| assert.deepEqual( mixedContents.not( [ childElements[ 0 ].nextSibling ] ).length, mixedLength - 1, "not textnode" ); |
There was a problem hiding this comment.
Suggestions on more rigorous tests are welcome, these seem a bit clumsy but of course it's never fun to manipulate non-element nodes in jQuery.
There was a problem hiding this comment.
Yeah, this is very fragile test, since it doesn't use generated fixtures.
Beside that, sounds like we need to test explicitly all node types? At least text and comment, maybe processing node too, not sure if we need to test/support deprecated nodes though.
It would be cool if it would be shown explicitly either through the code or with comment or through assertion name
|
Maybe another test exactly like from the ticket should be added too? http://jsbin.com/yobuhucife/1/edit?html,js,output hm, can it break user code? It seems it can :/. Hopefully not in the big way |
| } | ||
| if ( typeof qualifier !== "string" ) { | ||
| return jQuery.grep( elements, function( elem ) { | ||
| return ( indexOf.call( qualifier, elem ) > -1 ) !== not; |
There was a problem hiding this comment.
Additional parentheses are not required? Either way is good though
There was a problem hiding this comment.
Oh, i see we already use it like that, couple lines down
| if ( risSimple.test( qualifier ) ) { | ||
| return jQuery.filter( qualifier, elements, not ); | ||
| } | ||
| // Arraylike of elements (jQuery, arguments, Array) |
5 participants
Summary
Fixes gh-3226, was only +3 gzip.
The docs at http://api.jquery.com/not/ might need some wording changes to clarify that this is intended behavior?
Checklist
Mark an
[x]for completed items, if you're not sure leave them unchecked and we can assist.