Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
insertNodes
andinsertParagraph
#5002Fix
insertNodes
andinsertParagraph
#5002Changes from 69 commits
45f23de
dadfb30
539ec8f
79263fb
acee2ca
dbe5b51
eddff37
e18b4ed
b01827a
815831a
a8ecdb5
9321873
3c3adbe
a5c1f06
76021ad
5253c3f
f908afc
a5780cc
30ac722
9635b2b
3e27cf7
23bdfa3
650912e
a3eff0d
6770bbc
b34a521
46d6f20
61b2954
c75a97b
7126830
b2ac2c8
ce95063
240b675
05d742d
13f2ecf
2f550f2
16e410e
f6a763f
2e854d5
525e8e7
f9fea22
a4f87a1
4d10528
ae2634d
718af1e
6ca9021
f280884
48a9bb3
3fc0cb9
eb607a1
c84e4cb
f283828
591032a
fc01494
4deb414
fefc545
7de20be
396e313
d695f97
e81e054
c086bd3
0641539
7f29dee
d68853f
3c6a91e
34a1d47
5dc6563
6075bdb
87e6509
c3e9626
1c0b205
0f30d68
e04bb0f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one point there was a test that failed if an empty (unnecessary) string was added last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insertParagraph
callsinsertNewAfter
, andinsertNodes
callsinsertParagraph
. To simplify things and avoid bugs and circular references (very common while refactoring), CodeNode'sinsertNewAfter
no longer depends oninsertNodes
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So add what point would you draw the line between using
insertNodes
and having to handle it yourself? My first impression on the above code is that you're repeating the TextNode split logic present ininsertNodes
andinsertText
.Also, this logic is particularly complicated for anyone else to replicate, is there a possibility to have utilities for this?
One of the reasons why users use
insertNodes
heavily is to avoid having to split nodes at selection (otherwiseinsertAfter
would do)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InsertNodes is safe to use anywhere except in insertParagraph/insertNewAfter because of the circular dependency I mentioned.
What would be ideal is that insertNodes does not depend on insertParagraph. However, at this point I don't know if that's possible without implementing a long and convoluted logic, like what was used so far.
Much of the time I spent on this PR was trying to accomplish that. When I gave up, I would come up with some way that I thought it was possible, only to finally realize that it wasn't. Explaining it might be too long. But I can say that some tests that are reasonable make it horrible to achieve this ("Paste top level element in the middle of list" is perhaps the best example).
On your second question, I don't think there is a great benefit to reusing some utility here. What is here that is repeated in removeTextAndSplitBlock is only 3 lines of code actually (285-287). The rest is selection restoration (which is taken care of by insertNodes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that in insertNodes I am temporarily using the
__language in node
hack.What I would like to do in another PR is an "INSERT_NODES_COMMAND" that is used in @lexical/code. Then this code would not be necessary either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting from the PR description:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One detail that I noticed the new
$getAncestor
function improvedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be considered a bug fixed, since there is no need to insert an empty paragraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another improvement to insertNodes.
It doesn't make sense for the hr to be inserted after the list-item the cursor is over, if the cursor is at the beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expect was incorrect. This PR detected the bug and fixed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously
insertNewAfter
delegated toinsertParagraph
orinsertNodes
the responsibility of replacing the heading with a paragraph. By moving that responsibility here we can further simplifyinsertNodes
which is already too complex on its own.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thoughts as above even though in this case the resulting code is much simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
code
case is different from this one. There you avoid insertNodes. Here you avoid burdening insertNodes with the responsibility of replacing or transforming nodes.Actually, everything would be clearer if
insertNewAfter
was calledhandleEnter
.