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 support for overlapping marks #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

YousefED
Copy link

@YousefED YousefED commented Jun 8, 2021

This addresses issue #34 - while remaining backwards compatible.

There are still two TODO items open open:

  • the test currently fails, I think because there is a yChange attr on one of the documents. I'm not sure what the intention behind yChange is, so I'm not sure what the right way is to fix this
  • the current solution breaks YXmlText.toString because it doesn't handle arrays. I think we should add support for this directly in yjs, but want to make sure the solution I'm implementing here is a right approach.

/**
* @param {Y.XmlText} ytext
* @param {Array<any>} ptexts
*/
const equalYTextPText = (ytext, ptexts) => {
const delta = ytext.toDelta()
return delta.length === ptexts.length && delta.every((d, i) => d.insert === /** @type {any} */ (ptexts[i]).text && object.keys(d.attributes || {}).length === ptexts[i].marks.length && ptexts[i].marks.every(mark => equalAttrs(d.attributes[mark.type.name] || {}, mark.attrs)))
return delta.length === ptexts.length && delta.every((d, i) => d.insert === /** @type {any} */ (ptexts[i]).text && countYTextMarks(d.attributes || {}) === ptexts[i].marks.length && ptexts[i].marks.every(mark => containsEqualMark(d.attributes[mark.type.name] || {}, mark.attrs)))

Choose a reason for hiding this comment

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

Thanks for the PR for handling multiple marks!

I was pulling this PR into my project and ran into an issue with containsEqualMark. It looks like you're calling containsEqualMark with yattrs, pattrs instead of pattrs, yattrs.

IdrissMahjoubi pushed a commit to Collaborne/y-prosemirror that referenced this pull request Jun 16, 2022
This is inspired from this PL: yjs#52
@IdrissMahjoubi
Copy link

The first TODO is an open issue I think

the test currently fails, I think because there is a yChange attr on one of the documents. I'm not sure what the intention behind yChange is, so I'm not sure what the right way is to fix this

Check: #77 and #51

@vitmf
Copy link

vitmf commented Dec 20, 2022

Since ychange is lost in the conversion between prosemirror and ydoc, perhaps we should test the marks directly?

if (mark.type.name !== 'ychange') {

@vadimdemedes
Copy link

Hey @dmonad, I know it's an old PR, but could you please let us know if this is the right direction to take for supporting overlapping ProseMirror marks?

@piyush-kroolo
Copy link

@YousefED @dmonad

I know it's an old PR, but could you please let us know if this is the right direction to take for supporting overlapping ProseMirror marks?

+1

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.

6 participants