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

support marks on element nodes #157

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

support marks on element nodes #157

wants to merge 2 commits into from

Conversation

Pruxis
Copy link

@Pruxis Pruxis commented Jul 2, 2024

Issue 1: #47
Issue TipTap: ueberdosis/tiptap#4339

PR 1: #63
PR 2: #135

But updated so that it works to parse yDoc to JSON aswell.

I made sure that the tests run and formatting run green, if anything else is a concern feel free to let me know I will change as soon as possible

@nperez0111
Copy link

Hi @dmonad, as I understand it you wanted to add some tests for this. I had a hand in making this change and am willing to add tests if you find it necessary.

@Pruxis
Copy link
Author

Pruxis commented Jul 16, 2024

Anything pending to get this merged?

@aoruize
Copy link

aoruize commented Jul 16, 2024

Please get this merged 🙏🏼 🙏🏼 😭 😮‍💨

@arhamfhq
Copy link

Can somebody share any update we really need this issue fixed asap.

@nperez0111 nperez0111 force-pushed the master branch 2 times, most recently from cbfd93a to a00ae60 Compare November 22, 2024 14:07
@nperez0111
Copy link

I've added a couple of tests for this to prove that it works as intended.

The add test fails before this change, the remove test passes because the mark was never actually preserved in the ydoc

@dmonad
Copy link
Member

dmonad commented Dec 5, 2024

I'm sorry that I've been sitting on this for so long.

This PR makes quite a few changes that I can't take back easily. I know it fixes your immediate use-case. But longer-term, I believe that we should always go for the right solution.

I'm not happy with using Yjs attributes to sync marks. y-prosemirror reflects marks as formatting attributes. Hence, I believe the correct approach would be to use Yjs formatting attributes instead of attributes to sync marks.

There might be the argument that attributes and formatting attributes are semantically the same thing. If that were true, you could use prosemirror attributes instead of marks.

All I can say right now is that there is more to research here.

@nperez0111
Copy link

@dmonad do YXMLFragments support Yjs formatting attributes?

Do you think it would be difficult to implement in that way?

@dmonad
Copy link
Member

dmonad commented Dec 18, 2024

@nperez0111 Unfortunately, not yet. But I might be able to work on this next year.

@nperez0111
Copy link

@dmonad no worries!

If you need help at all, I'm happy to help. I've dived into the yjs codebase a couple of times now for things I needed. So maybe I can help move this forward somehow. I could dig around to understand the current implementation, but even a high-level description of how you'd see it working would be helpful

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.

5 participants