prosemirror/prosemirror-transform
1
0
Fork 0
Code Pull requests Activity

addMark allowing leaf nodes #22

Closed
cuppi wants to merge 1 commit from feat/add-mark-allow-leaf-node- into master
pull from: feat/add-mark-allow-leaf-node-
merge into: prosemirror:master
Conversation 8 Commits 1 Files changed 2 +3 -3
cuppi commented 2022-08-12 15:40:07 +02:00 (Migrated from github.com)
Copy link

Regarding this discussion( https://discuss.prosemirror.net/t/discussion-what-are-marks/862/16 ), I suggest addMark allowing leaf nodes, as I think it not affect the Mark model you mentioned.

Regarding this discussion( https://discuss.prosemirror.net/t/discussion-what-are-marks/862/16 ), I suggest addMark allowing leaf nodes, as I think it not affect the Mark model you mentioned.
marijnh commented 2022-08-12 15:43:33 +02:00 (Migrated from github.com)
Copy link

This would be a breaking change, and one that definitely is going to cause problems, so no, not going to merge this.

This would be a breaking change, and one that definitely is going to cause problems, so no, not going to merge this.
cuppi commented 2022-08-12 16:10:56 +02:00 (Migrated from github.com)
Copy link

@marijnh At present, we have encountered the problem that mark needs to be added on the block node.
However, setNodeMarkup will bring the mapping change, which will leads to the conflict merging problem in the collaborative scenario.
However, addMark has no problem. Therefore, we hope to reuse addMark & AddMarkStep as much as possible to solve the current problem.
Do you have any suggestions for us to reuse addMark or another solutions?
We look forward to your reply : )

@marijnh At present, we have encountered the problem that mark needs to be added on the block node. However, setNodeMarkup will bring the mapping change, which will leads to the conflict merging problem in the collaborative scenario. However, addMark has no problem. Therefore, we hope to reuse addMark & AddMarkStep as much as possible to solve the current problem. Do you have any suggestions for us to reuse addMark or another solutions? We look forward to your reply : )
marijnh commented 2022-08-13 12:50:24 +02:00 (Migrated from github.com)
Copy link

Does attached patch look like it would address this issue for you?

Does attached patch look like it would address this issue for you?
cuppi commented 2022-08-14 09:55:23 +02:00 (Migrated from github.com)
Copy link

Thanks for your reply.
Yes, it looks like it will solve my problem. In fact, I made a patch locally through a prototype chain rewrite, but I'm not sure if it will cause another problems that I haven't considered.
Do you think there will be any problems if we modify it according to this PR?

Thanks for your reply. Yes, it looks like it will solve my problem. In fact, I made a patch locally through a prototype chain rewrite, but I'm not sure if it will cause another problems that I haven't considered. Do you think there will be any problems if we modify it according to this PR?
marijnh commented 2022-08-14 11:11:54 +02:00 (Migrated from github.com)
Copy link

Do you think there will be any problems if we modify it according to this PR?

Do you mean merging this PR after all? As I said, that's not happening.

> Do you think there will be any problems if we modify it according to this PR? Do you mean merging this PR after all? As I said, that's not happening.
cuppi commented 2022-08-14 13:01:17 +02:00 (Migrated from github.com)
Copy link

NoNoNo, I just want to make sure that if I do this on my fork(NOT mergeing this PR), will it cause a problem?

NoNoNo, I just want to make sure that if I do this on my fork(NOT mergeing this PR), will it cause a problem?
marijnh commented 2022-08-14 13:32:41 +02:00 (Migrated from github.com)
Copy link

You'll likely see marks applied at multiple levels of your document tree, but other than that it might be unproblematic.

You'll likely see marks applied at multiple levels of your document tree, but other than that it might be unproblematic.
cuppi commented 2022-08-14 13:55:39 +02:00 (Migrated from github.com)
Copy link

Ok, I understand - The current implementation does not consider that Inline nodes may also have leaf nodes.
Thank you for your reply, which has taken up your time. : )

Ok, I understand - The current implementation does not consider that Inline nodes may also have leaf nodes. Thank you for your reply, which has taken up your time. : )

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
prosemirror/prosemirror-transform!22
No description provided.