Skip to content
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

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

tilgovi
Copy link
Contributor

@tilgovi tilgovi commented Jan 23, 2025

Remove the need to call useNodeView and render the portals manually. Make it happen automatically it <ProseMirror>.

Remove useNodeView from the public API.

@tilgovi tilgovi requested review from saranrapjs and a team January 23, 2025 23:34
@tilgovi tilgovi requested review from smoores-dev and a team as code owners January 23, 2025 23:34
@tilgovi
Copy link
Contributor Author

tilgovi commented Jan 23, 2025

This change would constitute a breaking change. I'm considering resetting main to what's on v1 right now and retargeting this against main.

@tilgovi
Copy link
Contributor Author

tilgovi commented Jan 23, 2025

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!

@tilgovi
Copy link
Contributor Author

tilgovi commented Jan 23, 2025

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.

@smoores-dev
Copy link
Collaborator

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 component property)

@tilgovi
Copy link
Contributor Author

tilgovi commented Jan 23, 2025

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 component property)

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.

@smoores-dev
Copy link
Collaborator

smoores-dev commented Jan 23, 2025

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.
@tilgovi tilgovi force-pushed the feat-automatic-react-node-views branch from 3788ee3 to 6f1a805 Compare January 23, 2025 23:57
node,
editorView,
getPos,
decorations,
innerDecorations
);

let componentRef: NodeViewWrapperRef | null = null;
const { component: NodeView } = nodeView;
if (!NodeView) {
Copy link
Member

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?

Copy link
Member

@saranrapjs saranrapjs left a 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?

@tilgovi
Copy link
Contributor Author

tilgovi commented Jan 24, 2025

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.

@tilgovi tilgovi merged commit f7cd3df into v1.x Jan 24, 2025
2 checks passed
@tilgovi tilgovi deleted the feat-automatic-react-node-views branch January 24, 2025 18:28
tilgovi added a commit that referenced this pull request Jan 24, 2025
Remove the need to call `useNodeView` and render the portals manually.
Make it happen automatically it `<ProseMirror>`.

Remove `useNodeView` from the public API.
tilgovi added a commit that referenced this pull request Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants