-
-
Notifications
You must be signed in to change notification settings - Fork 397
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
Update: ShadowRoot support #849
Update: ShadowRoot support #849
Conversation
…s & added arabic translation & updated past extension to support pasting images
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@dcadavez66 is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks. Two questions:
- can you move Arabic translations update and the Paste handler to separate PRs?
- Can you remove the commented out old code? We don't want the old code as comments in the codebase, and we can just see the differences here on github
After this we'll double check that all references to document
have indeed been updated to use view.root
instead
// document.body.addEventListener("drop", this.onDrop, true); | ||
this.pmView.root.addEventListener( | ||
"drop", | ||
this.onDrop as EventListener, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this work without the cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the cast no, there is another way where we explicitly type it with the type of the handler. Like so:
this.pmView.root.addEventListener(
"drop",
(event: Event) => this.onDrop(event as DragEvent),
true
);
Let me know the one you prefer to keep as standard and I can do the necessary updates!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we also conditionally add the listeners only if view.root instanceof Document
? Since shadow roots don't support listeners anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few changes that need to be made and we can merge:) Also make sure to check the comment regarding event listener type casting
// document.body.addEventListener("drop", this.onDrop, true); | ||
this.pmView.root.addEventListener( | ||
"drop", | ||
this.onDrop as EventListener, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we also conditionally add the listeners only if view.root instanceof Document
? Since shadow roots don't support listeners anyway
I added most of the changes mentioned in my review, only thing left is the type casting comment! @dcadavez66 @YousefED |
TODO:The changes related to the first point primarily involve replacing the use of document with the root property, which can be either Document or ShadowRoot. This property is accessed from pmView. In files where pmView is not available, the root property should be accessed from _editor.tiptapEditor.view.root.
To ensure consistency and ease for future modifications, there should be a standardized method for accessing this value. This will enable everyone making future changes to use this standard method instead of directly accessing the document.