Use a Plugin to assist in nesting node views according to node hierarchy #27
Conversation
smoores-dev
force-pushed
the
portal-tree-plugin
branch
from
e42a4d2
to
2bde75c
Compare
smoores-dev
force-pushed
the
portal-tree-plugin
branch
from
2bde75c
to
071c73d
Compare
src/hooks/useEditorView.ts
Outdated
| @@ -58,6 +58,7 @@ export function useEditorView<T extends HTMLElement = HTMLElement>( | |||
| props: DirectEditorProps | |||
| ): EditorView | null { | |||
| const [view, setView] = useState<EditorView | null>(null); | |||
| // const [keyRegistry]; | |||
There was a problem hiding this comment.
| // const [keyRegistry]; |
| element, | ||
| dom as HTMLElement, | ||
| Math.floor(Math.random() * 0xffffff).toString(16) | ||
| key |
There was a problem hiding this comment.
Is there any reason to have the portal registry key and the React key be different?
There was a problem hiding this comment.
The registration key isn't unique; all of the children of a given node will register with the same key. But they're not actually different; the react key is the key that the children of this node will register with. Does that make sense? Hoping I'm answering the right question haha.
There was a problem hiding this comment.
Ahh, right. Okay. And key is the registry key of this node. Maybe naming changes can help. We have a key for ourselves, and the key of the nearest ancestor, yeah?
Are the arguments here getting busy? Should we move createPortal here and then register that result?
There was a problem hiding this comment.
I think the naming I'm referring to is that "registry key" led me to think one key was a registry key and one key was a rendering key, but both are the same "kind" of key, the difference is which element they refer to.
There was a problem hiding this comment.
Agreed, the naming is not super clear! I'll try to see what I can come up with (I'll try registering just the portal, too, that makes sense)
smoores-dev
force-pushed
the
portal-tree-plugin
branch
from
38cfa38
to
b0c91e3
Compare
smoores-dev
force-pushed
the
portal-tree-plugin
branch
from
b0c91e3
to
d9c7c88
Compare
smoores-dev
force-pushed
the
portal-tree-plugin
branch
from
a735459
to
74a9346
Compare
|
Hey @tilgovi, curious about where your thinking is at on this one! |
|
I've pulled in the changes from |
| @@ -367,7 +367,7 @@ const reactNodeViews = { | |||
| const editorState = EditorState.create({ | |||
| schema, | |||
| // You must add the react if you use | |||
| // the useNodeViews hook. | |||
| // the useNodeViews or useNodePos hook. | |||
There was a problem hiding this comment.
Not strictly necessary; you can only use useNodePos if you're already using useNodeViews, since that's how you register React components as node views haha
There was a problem hiding this comment.
the useNodePos hook doesn't throw if the state field provided by this plugin isn't registered, so i feel like we want to note it somewhere? the reason i ran into the omission of this plugin in the first place was that we were using useNodePos and node updates were being silently eaten by ProseMirror because the node view position was off by way of that hook falling back to 0 when the react() plugin isn't found in state...
is there a different way you'd articulate the relationship between useNodePos and react()? or are you saying that as of this branch, using useNodeViews without react() won't work at all? if that's the case i wonder if we should take a harder line and throw (or something) when we can't find the plugin
There was a problem hiding this comment.
It won't throw, but it won't work properly (in that, it won't organize the React hierarchy correctly). I wouldn't be opposed to throwing if we can't find the plugin, though, I think that's probably a good idea actually! It should be pretty straightforward to do, since we're defining/have access to the plugin key!
All I was pointing out in my comment is that "using useNodeViews" already covers "using useNodePos" haha.
There was a problem hiding this comment.
in that, it won't organize the React hierarchy correctly
do things not render correctly? or do they just present incorrectly in React devtools?
There was a problem hiding this comment.
Uhhhh both I guess. Specifically, this PR makes it so that React Components of ProseMirror children are actually rendered as children of their parent nodes' React Components, which is important if you want context to flow correctly from parent to child. The issue that we were solving at dskrpt was for a collapsible detail/summary node, where we wanted to use context to pass parent state to the child.
So the DOM is rendered correctly either way, but since React hierarchy matters for context flow, things still don't work properly without the plugin.
|
I suspect that this is good to go; the good folks at https://dskrpt.de/ have been using this branch for months! Haha |
|
Haha unfortunately @saranrapjs you're not a codeowner, and I opened the PR, sooo we need someone else to approve :P |
|
I think I am now a code owner, but we have "Require approval of the most recent reviewable push" enabled (is why my approval doesn't count, I think). Will check in w/ some other folks in the morning! |
Alternative approach to #25. Instead of relying on stashing keys on dom nodes (which required queuing microtasks, since the dom reconciliation is still in progress during nodeview construction), this approach adds a ProseMirror Plugin that associates keys with nodes via their positions, and tracks node positions across transaction applications. The map from position to key allows node views to consistently find the key of their nearest React node view ancestor, since all node views have access to their own node's position.
This approach also adds sorting by position for child portals, so child element order is stable in the React tree and matches the order in the document.
Because this uses a Plugin with a
statekey, it can't be added by theProseMirrorcomponent (the way that thecomponentEventListenerPluginis, for example). Instead, this plugin is exported, and must be added by consumers. Everything will still work if consumers don't add this plugin; in fact, this change still an improvement even without the plugin, because of the stable sorting of React elements. But I've also updated the documentation to be very clear (and repetitive) about the need to use this plugin alongsideuseNodeViews.