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

✨ Add basic collaborative editing with automerge #71

Merged
merged 5 commits into from
Apr 27, 2023
Merged

✨ Add basic collaborative editing with automerge #71

merged 5 commits into from
Apr 27, 2023

Conversation

phlmn
Copy link
Member

@phlmn phlmn commented Apr 25, 2023

No description provided.

@phlmn phlmn force-pushed the phlmn/slate-automerge-set-paragraphs branch from 6f71de0 to cafaa88 Compare April 25, 2023 00:21
@phlmn phlmn requested review from pajowu, anuejn and rroohhh April 25, 2023 00:22
@anuejn anuejn mentioned this pull request Apr 25, 2023
Closed
@phlmn phlmn force-pushed the phlmn/slate-automerge-set-paragraphs branch 2 times, most recently from eabe3e7 to fe6083d Compare April 26, 2023 00:46
anuejn
anuejn approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@anuejn anuejn left a comment

Choose a reason for hiding this comment

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

Works like a charm :)

@anuejn anuejn force-pushed the phlmn/slate-automerge-set-paragraphs branch from fe6083d to 6a65762 Compare April 26, 2023 09:17
@phlmn
Copy link
Member Author

phlmn commented Apr 26, 2023

Works like a charm :)

Nice :)

But I don't get why the CI thinks the lockfile is outdated...

phlmn
phlmn commented Apr 26, 2023
View reviewed changes
frontend/pnpm-lock.yaml Outdated Show resolved Hide resolved
@anuejn anuejn force-pushed the phlmn/slate-automerge-set-paragraphs branch 3 times, most recently from c54f8d0 to f0dff42 Compare April 26, 2023 09:55
@anuejn anuejn enabled auto-merge April 26, 2023 09:56
@anuejn anuejn force-pushed the phlmn/slate-automerge-set-paragraphs branch 2 times, most recently from 6414f9f to cc6a0b1 Compare April 26, 2023 10:07
pajowu
pajowu approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@pajowu pajowu left a comment

Choose a reason for hiding this comment

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

Works great. LGTM, just two inline thoughts.

frontend/src/editor/automerge_websocket_provider.ts
@@ -1,4 +1,5 @@
import { Observable } from 'lib0/observable';
Copy link
Member

Choose a reason for hiding this comment

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

lib0 is still an old dependency from the yjs-devs. Does it make since to keep it now that we decided against yjs or can we switch this to something from the stdlib or self-built?

Copy link
Member Author

@phlmn phlmn Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh yes, this can be easily replaced with something self build. Would do this in a separate PR though

frontend/src/editor/slate_types.d.ts

declare module 'slate' {
interface CustomTypes {
Editor: BaseEditor & ReactEditor;
Editor: BaseEditor & ReactEditor & AutomergeEditor;
Copy link
Member

Choose a reason for hiding this comment

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

AutomergeEditor is already BaseEditor & ..., so the following should work?

Suggested change
Editor: BaseEditor & ReactEditor & AutomergeEditor;
Editor: AutomergeEditor & ReactEditor;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this should work. Do you like it better that way? :D I've no opinion on this

@phlmn
Copy link
Member Author

phlmn commented Apr 26, 2023

Seems like the lockfile of slate-automerge-doc was simply not up to date 🙈 Let's cross fingers the CI works now 🤞

@phlmn phlmn changed the title Add basic collaborative editing with automerge ✨ Add basic collaborative editing with automerge Apr 26, 2023
@phlmn phlmn force-pushed the phlmn/slate-automerge-set-paragraphs branch from 7c591dd to f9850be Compare April 26, 2023 11:36
@phlmn phlmn changed the title ✨ Add basic collaborative editing with automerge ✨ Add basic collaborative editing with automerge Apr 26, 2023
@phlmn phlmn disabled auto-merge April 26, 2023 11:51
@pajowu pajowu force-pushed the phlmn/slate-automerge-set-paragraphs branch from f9850be to 341d876 Compare April 26, 2023 18:22
@phlmn phlmn enabled auto-merge April 27, 2023 17:03
@phlmn phlmn added this pull request to the merge queue Apr 27, 2023
Merged via the queue into main with commit 657882e Apr 27, 2023
@phlmn phlmn deleted the phlmn/slate-automerge-set-paragraphs branch April 27, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anuejn anuejn anuejn approved these changes

@pajowu pajowu pajowu approved these changes

@rroohhh rroohhh Awaiting requested review from rroohhh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@phlmn @pajowu @anuejn