ded / bonzo

library agnostic, extensible DOM utility
Other
1.32k stars 137 forks source link

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

Open ColemanGariety opened 11 years ago

ColemanGariety commented 11 years ago

...for the moment.

ded commented 10 years ago

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

rvagg commented 10 years ago

@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 commented 10 years ago

(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)

ColemanGariety commented 10 years ago

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

rvagg commented 10 years ago

@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.

ColemanGariety commented 10 years ago

@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?