-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat!: automatically use node view components #158
Conversation
This change would constitute a breaking change. I'm considering resetting |
There's also part of me that's worried that @smoores-dev and I have talked through doing this before and decided not to for some reason I can't remember. If so, please help me remember! |
This PR also fixes #157 because it makes it so that the second, forced render after mounting the editor view includes the initial node view portals. |
afd0833
to
0cf126c
Compare
I think that this was mostly a desire to avoid coupling things while we were still solidifying the API. That said, does this change as is still work with non-React nodeViews? It seems that we would want to expose two node view props, or distinguish between types of node views by inspecting their props (i.e., looking for a |
This change does exactly what you describe regarding distinguishing between node view types. It wraps all the node view constructors, but ends up wrapping it with the identity function if the node view constructor returns a node view without a component property. |
Oh I see! I had missed the new branch in createReactNodeViewConstructor. Awesome :) |
Remove the need to call `useNodeView` and render the portals manually. Make it happen automatically it `<ProseMirror>`. Remove `useNodeView` from the public API.
3788ee3
to
6f1a805
Compare
node, | ||
editorView, | ||
getPos, | ||
decorations, | ||
innerDecorations | ||
); | ||
|
||
let componentRef: NodeViewWrapperRef | null = null; | ||
const { component: NodeView } = nodeView; | ||
if (!NodeView) { |
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.
worth adding a comment here to note that this is the branch which continues to allow specifying non-React nodeviews?
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.
This looks good to me! I am assuming (but have not run myself to confirm) that this passes the failing widget decoration test — is that worth adding here, too?
I checked that it passed, but I'll commit it separately. |
Remove the need to call `useNodeView` and render the portals manually. Make it happen automatically it `<ProseMirror>`. Remove `useNodeView` from the public API.
Remove the need to call
useNodeView
and render the portals manually. Make it happen automatically it<ProseMirror>
.Remove
useNodeView
from the public API.