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

Add CommentPlugin #2021

Merged
merged 10 commits into from
Apr 29, 2022
Merged

Add CommentPlugin #2021

merged 10 commits into from
Apr 29, 2022

Conversation

trueadm
Copy link
Collaborator

@trueadm trueadm commented Apr 28, 2022

This adds the CommentsPlugin to the playground. There are few things to point out:

  • It doesn't work with collaboration.
  • MarkNodes can be buggy and need refining.
  • You can't edit comments.

@vercel
Copy link

vercel bot commented Apr 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Apr 29, 2022 at 1:56PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Apr 29, 2022 at 1:56PM (UTC)

trueadm added 2 commits April 28, 2022 14:40
WIP

WIP

WIP

WIP

WIP

WIP
@fantactuka
Copy link
Contributor

Content marks is the trickiest part, but looks great anyways 👍

Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing!

Feedback:

  • I like the use of an ElementNode to mark these, especially since they have to be synced over collab.

  • The use of reference vs comment is confusing. Why is it called CommetNode but you're instead creating a Reference with a quote in the plugin?

  • Can I ever delete a reference? How would that work UX wise? I was expecting the reference to be deleted once there are no comments.

  • We should prevent creating a new reference on the same exact reference. In fact, I find it a bit confusing that the reference quote never updates. I guess it's good for the historical context but the fact that I can create multiples with the same text and that the node on the editor can potentially point to a completely different text is confusing.

  • How are we thinking about collab given that the information is currently decouple from the EditorState? Are we expecting the app to have their own mechanism for these or is this something we want to iterate on later?

  • If I delete a comment and undo:

Uncaught Error: Cannot call set() on a frozen Lexical node map

packages/lexical-playground/src/plugins/CommentPlugin.jsx Outdated Show resolved Hide resolved
packages/lexical-playground/src/plugins/CommentPlugin.jsx Outdated Show resolved Hide resolved
@trueadm
Copy link
Collaborator Author

trueadm commented Apr 29, 2022

Addressed your feedback!

How are we thinking about collab given that the information is currently decouple from the EditorState? Are we expecting the app to have their own mechanism for these or is this something we want to iterate on later?

There will be a separate model for CommentPlugin's state that will be represented in another Yjs doc. I'll do that next week when I get some time, as it's quite a lot of work.

@trueadm trueadm merged commit b84076e into main Apr 29, 2022
@trueadm trueadm deleted the comments branch April 29, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants