search

dfilatov / vidom

Library to build UI based on virtual DOM
MIT License
415 stars 16 forks source link

text nodes are only child, not parent #94

Closed ghost closed 9 years ago

ghost commented 9 years ago

A..

dfilatov commented 9 years ago

TextNode are already attached to it - rendered.

How is text node already attached? I can't understand how node can be attached without appending to its parent.

in the patch method for the TextNode, you are checking type against type. This is code will never be executed. Just do a console.log on it, and it will return nothing.

Just try to patch createNode('div').children(createNode().text('text')) with createNode('div').children(createNode('span')) and you'll see code is executed.

why do you want to set current parent namespace for a TextNode? It will not help you, a text is nodeType 3 and can't be attached to anythng then text.

Seems you're right. I'll find out and fix it.

Again for the patching part for the TextNode. It will be better performance if you use textContent if the node are attached to it's parent, and nodeValue the other way around.

I don't understand. How is using of textContent or nodeValue related to the fact of attaching to its parent?

dfilatov commented 9 years ago

comparing two text nodes before using appendChild

There's no user way to compare nodes before they are attached to DOM.

For the appendChild() method, just try it. Try to avoid render a text node, and you will see it still works.

If I avoid rendering of a text node what I should append to its parent DOM node?

dfilatov commented 9 years ago

mount() isn't for attaching child nodes. It's not for dealing with DOM operations.

dfilatov commented 9 years ago

Because all types of nodes should have common interface to get rid of redundant duck type checking.

dfilatov commented 9 years ago

why do you want to set current parent namespace for a TextNode? It will not help you, a text is nodeType 3 and can't be attached to anythng then text.

I have investigated and haven't found where I set "current parent namespace for a TextNode". Could you point?

dfilatov commented 9 years ago

parent is set not to keep its ns, it is set to make possible "replace" operation. See https://github.com/dfilatov/vidom/blob/master/lib/nodes/TextNode.js#L43

What about https://github.com/ractivejs/ractive/blob/dev/src/virtualdom/items/Text.js#L17-L22. As I have already written many times, in vidom it's impossible sutiation because user isn't dealing with render process, user can't call renderToDom directly. It's not a part of user interface.

dfilatov commented 9 years ago

The line I was linking to in your code. Comment out the parent check, and run test cases again. Can you tell me why the tests didn't break?

Because there's no such test. I'll add it.

ghost commented 9 years ago

Aha!

dfilatov commented 9 years ago

Now vidom uses cloneNode for creating DOM nodes.