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.

Sign up for GitHub

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

Jump to bottom

Fix nested editors in collab #2781

Merged
merged 5 commits into from
Aug 10, 2022
Merged

Fix nested editors in collab #2781

merged 5 commits into from
Aug 10, 2022

Conversation

trueadm
Copy link
Collaborator

@trueadm trueadm commented Aug 7, 2022 •
edited
Loading

Fixes #2779. We now use a boolean flag to determine if collab is ready and active.

@vercel
Copy link

vercel bot commented Aug 7, 2022 •
edited
Loading

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Aug 10, 2022 at 6:25PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Aug 10, 2022 at 6:25PM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 7, 2022
@echarles
Copy link
Contributor

echarles commented Aug 7, 2022

Thx @trueadm, just tested on https://lexical-playground-git-fix-collab-nested-editors-fbopensource.vercel.app/split/index.html?isCollab=true

The sticky notes content are synced, but the equation do not sync for me.

@echarles
Copy link
Contributor

echarles commented Aug 7, 2022

The image caption is also synced.

These changes looks to correctly address the nested editor case, but the equation editor is another case that does not use the nested editor.

A general question for me is "What are the requirements to make a Decorator node syncable?"

@trueadm
Copy link
Collaborator Author

trueadm commented Aug 7, 2022 •
edited
Loading

A general question for me is "What are the requirements to make a Decorator node syncable?"

Decorator nodes should sync automatically. If you use the Lexical primitive values, it should "just work". I'll take a look at the equation editor, but it's likely a regression in that plugin somewhere along the lines too.

Update: was a simple bug in the equation node logic, it wasn't updating the React state with the value on the node. Added the fix to the same PR.

@acywatson
Copy link
Contributor

Do we have any tests around this? Might be nice to add that so this doesn't regress again.

@echarles
Copy link
Contributor

echarles commented Aug 8, 2022

Decorator nodes should sync automatically. If you use the Lexical primitive values, it should "just work".

Thx. I will dig more into the code to better understand the logic and hopefully come with a PR with some documentation on that.

Update: was a simple bug in the equation node logic, it wasn't updating the React state with the value on the node

Equation are now synced in the playground. Thx.

fantactuka
fantactuka approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@fantactuka fantactuka left a comment

Choose a reason for hiding this comment

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

Looks great, completely missed that we've used main as collab indicator for plugins

fantactuka
fantactuka reviewed Aug 8, 2022
View reviewed changes
packages/lexical-react/src/__tests__/unit/utils.tsx

const [editor] = useLexicalComposerContext();

const {yjsDocMap} = context;
context.isCollabActive = true;
Copy link
Contributor

@fantactuka fantactuka Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should this have useEffect to unset isCollabActive on onmount?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Mind taking over this PR? I won’t be able to work on it again till I’m back in a few weeks.

@fantactuka fantactuka merged commit 7052334 into main Aug 10, 2022
@fantactuka fantactuka deleted the fix-collab-nested-editors branch August 10, 2022 18:53
thegreatercurve pushed a commit that referenced this pull request Nov 25, 2022
@trueadm @fantactuka @thegreatercurve
Fix nested editors in collab (#2781)
59bbd76 
* Fix nested editors in collab

* Better fix

* Better fix

* Fix equations

* Reset is-collab flag when root-level collab plugin unmounts

Co-authored-by: fantactuka <fantactuka@gmail.com>
@thegreatercurve thegreatercurve mentioned this pull request Nov 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fantactuka fantactuka fantactuka approved these changes

@acywatson acywatson acywatson approved these changes

@zurfyx zurfyx Awaiting requested review from zurfyx zurfyx is a code owner

@tylerjbainbridge tylerjbainbridge Awaiting requested review from tylerjbainbridge

@thegreatercurve thegreatercurve Awaiting requested review from thegreatercurve

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: DecoratorNodes content should be collaborative
5 participants
@trueadm @echarles @acywatson @fantactuka @facebook-github-bot