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

fix(core): implement deleteCurrentNode command & fix node joining on Delete key #3192

Conversation

bdbch
Copy link
Contributor

@bdbch bdbch commented Sep 13, 2022

This PR should fix #2924 by adding a new command called deleteCurrentNode.

This command is then used inside the keymap core extension right before the joinForward command to check if the current node is empty - if it is, the current node will be simply deleted instead of joining the empty node with the next node.

This way, if for example an empty paragraph node is followed by an blockquote, the empty paragraph node will be removed and the selection will be moved into the blockquote.

If the paragraph node is not empty, the blockquote will be transformed into a paragraph node as it used to be.

This PR implements a "deleteCurrentNode" action in combination with registering this command inside the keymap for the delete key. This way, we editor will check, if the current node is empty before joining - if the current node is empty, the node will be removed. Joining will still work if the current node is not empty and the selection is at the end of the current node

2924
@bdbch bdbch added this to the 2.0.0 milestone Sep 13, 2022
@bdbch bdbch self-assigned this Sep 13, 2022
@netlify
Copy link

netlify bot commented Sep 13, 2022

Deploy Preview for tiptap-embed failed.

Name Link
🔨 Latest commit 4efbfef
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/6380e0409fab2300088d08f5

@rfgamaral
Copy link
Contributor

@bdbch This is awesome, thanks for taking the time to dig into this one.

However, while attempting to fix this on my side (I couldn't), I found an edge case that I'm not sure if this implementation takes care of. The deployment seems to have failed, but I'll try to find some time today to check out this branch, and test it out. I'll let you know what I find as soon as possible.

@bdbch
Copy link
Contributor Author

bdbch commented Sep 13, 2022

Thanks for your response @rfgamaral! I'm taking a look into the failed deployment right now.

@bdbch
Copy link
Contributor Author

bdbch commented Sep 13, 2022

Seems like the Netlify cache was breaking the build. It's deployed now. Feel free to test here:
https://6320b6c00a7e9271f33a732c--tiptap-embed.netlify.app/src/examples/book/react/

@bdbch
Copy link
Contributor Author

bdbch commented Sep 13, 2022

What edge case did you find?

@rfgamaral
Copy link
Contributor

@bdbch This one:

firefox_mTdG8AWAPO.mp4

Based on my experience with other editors, in this particular case, the blockquote node (or other block node like a list item) should indeed be removed, and the text joined with the paragraph where the cursor was.

Short demo from Google Docs:

firefox_59sdQyvkQN.mp4

Hope you can fix this one as well 🤞

@bdbch
Copy link
Contributor Author

bdbch commented Sep 13, 2022

I'll take a look at this case too!

@bdbch
Copy link
Contributor Author

bdbch commented Sep 14, 2022

@rfgamaral sorry didn't find time yesterday, was quite late already haha. I'll take a look at my PR later this day and I'll see if I can find a quick solution for the issue you showed me.

@rfgamaral
Copy link
Contributor

@bdbch That's okay, I understand, take the time required to get it right 👍

@bdbch bdbch changed the title Implement deleteCurrentNode command & fix node joining on Delete key fix(core): implement deleteCurrentNode command & fix node joining on Delete key Sep 22, 2022
…g-delete-on-an-empty-paragraph-clears-the-node-afterwards
@bdbch
Copy link
Contributor Author

bdbch commented Sep 30, 2022

@rfgamaral I worked on this a bit but couldn't figure out the "right" way to do it. What I'm on right now:

  1. Get to the parent node (for example the paragraph)
  2. Look for the next text node found after the parent node (for example in the next paragraph, list item, etc)
  3. Copy the text to the end of the current cursor $anchor position
  4. IF the next node is not a text node, we will skip and merge the next node as usual
  5. Remove the parent of the copied text node

But I'm not 100% sure if this is the expected behaviour. I'm still experimenting - just a heads up for what is going on in here.

bdbch and others added 5 commits September 30, 2022 05:15
…g-delete-on-an-empty-paragraph-clears-the-node-afterwards
…g-delete-on-an-empty-paragraph-clears-the-node-afterwards
…de-afterwards' of github.com:ueberdosis/tiptap into 2924-pressing-delete-on-an-empty-paragraph-clears-the-node-afterwards
…g-delete-on-an-empty-paragraph-clears-the-node-afterwards
@bdbch
Copy link
Contributor Author

bdbch commented Nov 25, 2022

@rfgamaral since this PR takes longer I'll merge it soon and release it with the next release. I created a new issue for the edgecase you reported: #3454

@bdbch bdbch merged commit ef8a1a2 into main Nov 25, 2022
@bdbch bdbch deleted the 2924-pressing-delete-on-an-empty-paragraph-clears-the-node-afterwards branch November 25, 2022 15:42
Comment on lines +6 to +9
/**
* Delete the node that currently has the selection anchor.
*/
deleteCurrentNode: () => ReturnType,
Copy link
Contributor

Choose a reason for hiding this comment

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

This command should be marked @internal or at least have its behavior documented more clearly. I tried using it thinking it would delete the current node, but the current node was not empty, so it wasn't deleted, leading me to think Tiptap was broken.

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.

Pressing Delete on an empty paragraph clears the node afterwards
3 participants