Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Feature/145 #150

Merged
merged 7 commits into from
Aug 24, 2016
Merged

Feature/145 #150

merged 7 commits into from
Aug 24, 2016

Conversation

MobiusHorizons
Copy link
Contributor

Fixes #145

@developit
Copy link
Member

I think this makes sense!

@developit
Copy link
Member

developit commented Aug 19, 2016

Only thing we'll need is to have it also remove extra elements that may have been contained in parent on initial render. Right now it leaves the <p> in the example below:

<div id="app">
  <section id="root">to be rendered by preact-compat</section>
  <p>should be removed since it's not in the vtree</p>
</div>
render(<section id="root">rendered</section>, $('#app'));
console.log( $('#app').children.length );  // 2, should be 1

(or at least this is my understanding of what React seems to do)

@MobiusHorizons
Copy link
Contributor Author

@developit, I have added a test that I believe covers this case. Please see the updated code.

@developit
Copy link
Member

Might be an issue with multiple node removal here. .children is a Live NodeList and so calling removeChild() on the second child actually shifts the list :o

I don't mind tweaking it after merge if you're getting the desired result though, I really appreciate your work on this!

@MobiusHorizons
Copy link
Contributor Author

Good point, I will fix it. I also realized that textnodes in the parent wouldn't be taken care of, since they don't show up as children.

@developit
Copy link
Member

Indeed! Switching to childNodes should fix that.

@MobiusHorizons
Copy link
Contributor Author

Alright, I think this covers my use cases. tweak as necessary.

@developit
Copy link
Member

Looks great, I'll merge as soon as I have a sec!

@MobiusHorizons
Copy link
Contributor Author

cool, glad to hear it.

@developit developit added this to the 2.4 milestone Aug 22, 2016
@chiragmongia
Copy link
Contributor

I would love to see this getting merged! 😃

@developit developit merged commit 913696a into preactjs:master Aug 24, 2016
developit added a commit that referenced this pull request Aug 24, 2016
@MobiusHorizons MobiusHorizons deleted the feature/145 branch August 24, 2016 14:56
@developit
Copy link
Member

Released as 3.0.0! 🎉

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

Successfully merging this pull request may close these issues.

None yet

3 participants