-
Notifications
You must be signed in to change notification settings - Fork 45
Some initial work on React support #8
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
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: