Conversation
f087f4e to
af00c3c
Compare
|
Some thoughts on this PR:
|
6680c39 to
12e9421
Compare
|
|
||
| // Decorate the container element with styles that will match | ||
| // If using React, the element object turns into a props object. | ||
| if (config.isReact) { |
There was a problem hiding this comment.
Hmm, I'm not fond of those conditions. I'm sure that there is a way to manage vanillajs and react with the same code no?
There was a problem hiding this comment.
Yup, I agree. For now this allows it to work so I can move forward, but I don't think we should merge this as-is.
There was a problem hiding this comment.
I'll give this a shot this weekend and let you know my findings.
I don't really like all the isReact-specific conditionals necessary.
I didn't read this, but yeah adding framework-specific code isn't that clean :p
| return config.applyPatch(element, fragment, scroller) | ||
| } | ||
|
|
||
| element.innerHTML = '' |
There was a problem hiding this comment.
If I understand this correctly, React is not DOM-friendly? I mean that for react you need some kind of virtual dom based on objects? Isn't there a way to transform real DOM elements to react-like dom outside this script?
There was a problem hiding this comment.
There are, like React Faux DOM, but this performs significantly better and allows for a cleaner wrapper to built in React. We might want to approach this as just being VDOM-friendly and not focusing on React-specifically. Like diffHTML could benefit from these changes as well, but allow its VTree representation of a DOM Node to pass through HyperList and get the right positional values associated with it.
This is much more usable as a wrapped React component, consumption looks like:
With the experimental implementation being: