bonzo icon indicating copy to clipboard operation
bonzo copied to clipboard

#126: Fixed replaceWith(). Works as expected, but tests are failing...

Open JacksonGariety opened this issue 10 years ago • 6 comments

...for the moment.

JacksonGariety avatar Nov 13 '13 22:11 JacksonGariety

oh man i feel silly for implementing this myself when this pull request was sitting here. tests still fail for me for replaceWith. @rvagg any thoughts on whether the tests are wrong or the implementation is wrong

ded avatar Feb 22 '14 21:02 ded

@ded, implementation is buggy.

original:

         return bonzo(this[0].parentNode.replaceChild(bonzo(normalize(node))[0], this[0]))

new:

        var that = this
        return this.each(function (el, i) {
          each(normalize(node, that, i), function (i) {
            el[parentNode] && el[parentNode].replaceChild(i, el)
          })
        })

This test case is passing in an array with multiple elements and the new loop is now calling replaceChild() multiple times on the same parent node with the same el--so the first replacement happens and then subsequent calls do nothing because el has already been replaced.

The fix is ... I don't know but you want to replace the child with a bunch of elements at once, not do a replacement for each element.

rvagg avatar Feb 22 '14 21:02 rvagg

(also, I still think the test is good so just make it pass and there will be world-peace and free puppies for every child)

rvagg avatar Feb 22 '14 21:02 rvagg

@rvagg So the first element in the passed array should replace, and the subsequent elements should be appended after?

JacksonGariety avatar Feb 23 '14 01:02 JacksonGariety

@JacksonGariety no, the whole array should replace whatever you're replacing. Even if it's three elements replacing one, I think. It's worth going off jQuery for these APIs because they aren't intuitive in the complex cases but people expect them to act in a certain way, because jQuery.

rvagg avatar Feb 23 '14 06:02 rvagg

@rvagg but replaceChild only replaces a single element. And in theory, replacing the first one and appending the subsequent ones after the first would be identical, no?

$('foo')replaceWith(['<bar/>', '<raz/>'])

<wat/><foo/><wat/> would become <wat/><bar/><raz/><wat/>

Right?

JacksonGariety avatar Feb 23 '14 07:02 JacksonGariety