Handle the case of an empty string. Fixes #13401. #1163

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@markraddatz
Contributor

Adds a value === "" check to .replaceWith().

@dmethvin

Can just be return this.remove(); I think?

Wondering whether we should be catching other falsy values rather than passing them to domManip. Also now that I look at it, most of the other appending methods allow multiple args but we don't here. Kinda strange.

@gibson042 gibson042 commented on an outdated diff Feb 7, 2013
src/manipulation.js
@@ -241,6 +241,12 @@ jQuery.fn.extend({
value = jQuery( value ).not( this ).detach();
}
+ if ( value === "" ) {
+ return this.each(function() {
+ jQuery( this ).remove();
+ });
+ }
+
return this.domManip( [ value ], true, function( elem ) {
@gibson042
gibson042 Feb 7, 2013 Member

Why not:

return value === "" ?
    this.remove() :
    this.domManip(…);

?

@gibson042
Member

All true, @dmethvin. .domManip does some weird input handling, casting true to text but ignoring false (shades of #10867? 😈), and .replaceWith is the black sheep of manipulation methods. I'd like to fix both in 1.11/2.1.

@gibson042 gibson042 closed this in 6a0ee2d Feb 8, 2013
@gibson042
Member

Thanks @markraddatz; I made some minor tweaks for style and compression but the fix is all yours!

@gibson042 gibson042 added a commit that referenced this pull request Feb 8, 2013
@markraddatz @gibson042 markraddatz + gibson042 Fix #13401: replaceWith(""). Close gh-1163.
(cherry picked from commit 6a0ee2d)
03ab9b9
@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@markraddatz @gibson042 markraddatz + gibson042 Fix #13401: replaceWith(""). Close gh-1163.
(cherry picked from commit 6a0ee2d)
c481c11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment