Read and reassign selected text marks when editing a link #238
Conversation
|
@maatMa Thanks for looking into this. You mentioned here #237 (comment)
Is that still true of this solution? I haven't had a chance to test this out. At first glance, this seems like it may work in a large number of cases, though I'm not sure if it will work 100% of the time. For instance, if the link has part of the text in bold and part not, what will happen to the resulting link? Based on the code, I'd guess it'll end up being either all bold or all not bold, even though users will likely expect the original styling to be preserved. Tricky, and still perhaps an improvement over the behavior today! Also side note that the TS type-checking failed on this branch (with a minor issue related to Tiptap generics); see the build result. |
|
Yes, the Text Edit Problem is still the case. I can't think of a good soulution there.
Multiple marks are supported, the function reads every existing text mark and reassign it to the edited link. I allready tested this with multiple marks and it looks like it works. Thats why it is so hard to implement the text changes. I don't know how to assign the changed text inside the mark array. |
|
I tried to fix the Typescript errors. Looks like I've got some more issues in my code regarding the mapping between the |
|
Thanks to @semanticist21 the text update is working now. |
|
@sjdemartini Is this Pull request fine, or do you need some more intormations, testing, refactoring, etc. |
|
@maatMa I'll play around with this when I get some time and will follow up once I do. As you've seen through a couple of the iterations here, link editing/creation is quite finicky in Tiptap, so I'd like to make sure there isn't any regression in behavior. |
|
@sjdemartini Sure, no problem. Just wanted to make sure you're not waiting for me. |
|
@sjdemartini Did you had time to test the pull-request? |
|
@maatMa Sorry for the delay. I haven't had time to test this out yet but plan to soon. (I want to be thorough to try to ensure we avoid any bugs, since I've found this to be a delicate piece of functionality.) Thanks for your patience. |
sjdemartini
changed the title
There was a problem hiding this comment.
Thanks again for the work on this and your patience. I haven't had a chance to try to run through various scenarios and test this extensively/manually since I have a lot of other things on my plate. I think it will probably be worth adding unit tests for this sort of functionality, given how many edge cases and scenarios there are, and how brittle this sort of behavior in Tiptap can be. If only setLink() were sufficient!
By the way, Tiptap just recently added somewhat similar logic related to toggling the heading type, where they copy over / preserve attributes ueberdosis/tiptap#5489, which may be useful to reference. Maybe we can use similar logic here? Their unit-tests may also be handy as a place to get started.
| const resolvedPos = state.doc.resolve(selection.from); | ||
| const nodeAfter = resolvedPos.nodeAfter; | ||
|
|
||
| if (nodeAfter?.isText) { |
There was a problem hiding this comment.
Why is it we only use this logic below if specifically the next node is text?
And what if there are multiple nodes within the selection, not just two?
Pull request related to the issue #237