[0.8] List indentation simplified

[GermanJablo]

In $handleIndent and $handleOutdent the Github diff gets messy with line mapping. The only thing I did there was remove the for loop, and that's why the indentation of the whole function changed.

In fact, literally all I did in this PR was remove 100 unnecessary lines of code. Everything is still working fine.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 2, 2023 at 2:17AM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 2, 2023 at 2:17AM (UTC)

[thegreatercurve]

Thanks for the PR! I understand your point about simplicity, but I'm not sure if this introduces much value other than introducing a major breaking change. There's also some value in supporting the current implementation which accepts an array of ListItemNodes.

[GermanJablo]

@thegreatercurve It will not be a breaking change for anyone. The only function that was exported was indentList, which is useless called by itself.

This PR is not just about simplicity, and it does add a lot of value.

• I have a specific use case where I had to copy the handleIndentAndOutdent function to my repository to overwrite it, only to later realize that it didn't work on lists because for no reason they are handled elsewhere. To fix this I would have to end up copying a bunch of internal Lexical functions, which also have no reason to exist.
• I lost many hours of work on Fix converting nodes with nested lists #3821 for the same problem. The very long traversal that is done to indent lists makes it very difficult to debug and is prone to bugs.
• The new implementation also has a big improvement when it comes to performance (see below).

And finally, there is actually no value in supporting the current implementation which accepts an array of ListItemNodes. This is not used throughout the repository. If you pay attention, this is what you guys are doing:

1. Save in an array all the nodes of the selection.
2. Indent all except listItem.
3. Save in another array all the nodes of the selection again.
4. Filter that array to get only the listItem.
5. Indent those listItem.

The most curious of all, is that the same method to indent from step 2 works for the listItem, I didn't even have to modify the function.

I consider it was my mistake not to add a more detailed explanation of the reasons and benefits of this PR.

[thegreatercurve]

Apologies @egonbolton! My mistake for not looking properly at the exports. Thank you for the detailed explanation above though. I can see that it does add a lot of value, especially for performance.

I know the exported methods are probably rarely used by themselves, but we'll roll this into version 0.8, just because it could constitute a breaking for any one who does use them.

[acywatson]

@thegreatercurve It will not be a breaking change for anyone. The only function that was exported was indentList, which is useless called by itself.

This PR is not just about simplicity, and it does add a lot of value.

• I have a specific use case where I had to copy the handleIndentAndOutdent function to my repository to overwrite it, only to later realize that it didn't work on lists because for no reason they are handled elsewhere. To fix this I would have to end up copying a bunch of internal Lexical functions, which also have no reason to exist.
• I lost many hours of work on Fix converting nodes with nested lists #3821 for the same problem. The very long traversal that is done to indent lists makes it very difficult to debug and is prone to bugs.
• The new implementation also has a big improvement when it comes to performance (see below).

And finally, there is actually no value in supporting the current implementation which accepts an array of ListItemNodes. This is not used throughout the repository. If you pay attention, this is what you guys are doing:

1. Save in an array all the nodes of the selection.
2. Indent all except listItem.
3. Save in another array all the nodes of the selection again.
4. Filter that array to get only the listItem.
5. Indent those listItem.

The most curious of all, is that the same method to indent from step 2 works for the listItem, I didn't even have to modify the function.

I consider it was my mistake not to add a more detailed explanation of the reasons and benefits of this PR.

This seems like a good change, but you have a ton of changes in the diff that are basically non-sensical/unrelated to the thing you're trying to address. It makes it way easier to review these things if you can limit the diff to the relevant changes.

Also, as you yourself pointed out, if you're going to propose a change like this, you really need to have a full explanation of why you are making the change and what exactly it is. The same goes for your other PR, where you're, for instance, making a bunch of changes to a core function in the reconciler with no explanation as to why they are necessary in the PR description.

[GermanJablo]

Wow, I'm a little surprised. I know in another PR there were a couple of small changes that could have gone in a different PR.

Could you tell me in this case what are those ton of changes in the diff that are basically non-sensical/unrelated to the thing I'm trying to address? I honestly don't see any.

I've also rechecked all my PRs, and haven't found any changes to a core function of the reconciler, so I don't know what you mean by that.

My sincere apologies if I made another mistake that I'm not seeing that wasted your time. I'm not justifying myself, I just want to learn.

[acywatson]

Here's an example

[GermanJablo]

I'm not sure what you mean here. As I said in the PR description:

In $handleIndent and $handleOutdent the Github diff gets messy with line mapping. The only thing I did there was remove the for loop, and that's why the indentation of the whole function changed.

In these lines the only thing that changes is an indent, because it is not inside a for loop.

[acywatson]

Yea it's fine - this is good work. Let's get the e2e tests passing and get this merged!

[acywatson]

Here's another one

[acywatson]

Wow, I'm a little surprised. I know in another PR there were a couple of small changes that could have gone in a different PR.

Could you tell me in this case what are those ton of changes in the diff that are basically non-sensical/unrelated to the thing I'm trying to address? I honestly don't see any.

I've also rechecked all my PRs, and haven't found any changes to a core function of the reconciler, so I don't know what you mean by that.

My sincere apologies if I made another mistake that I'm not seeing that wasted your time. I'm not justifying myself, I just want to learn.

@egonbolton

Sorry, re-reading my comment, I think it sounded harsher than I intended. You aren't wasting my time - quite the opposite. I really appreciate the effort you've put into these PRs. I was just trying to give some thoughts on how to make it easier/quicker for us to review, since it usually takes us way too long to do that (and that's not you or the communities fault).

I think the problem with the diff is all the whitespace changes and the slight movements of code that are causing it to look like you changed a lot more than you did. I see that we only "deleted 100 lines of code" but at the same time there are many more lines than that affected in the code diff.

On the reconciler change, we can take that conversation over to the other PR, but my thoughts were more about just explaining the intent. The function I had in mind was removeFromParent, which is actually in LexicalUtils, not the reconciler module.