Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JacksonGariety
Copy link
Contributor

...for the moment.

@ded
Copy link
Owner

ded commented Feb 22, 2014

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
Copy link
Collaborator

rvagg commented Feb 22, 2014

@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
Copy link
Collaborator

rvagg commented Feb 22, 2014

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

@JacksonGariety
Copy link
Contributor Author

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

@rvagg
Copy link
Collaborator

rvagg commented Feb 23, 2014

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

@JacksonGariety
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants