Accept non-text nodes in insertTextByKey

[oyeanuj]

Hi @ianstormtaylor, as per your comments in #775, this function now accepts a non-text node and calculates the leaf text node at the offset, and then proceeds to insert text.

[ianstormtaylor]

Yay! Great work @oyeanuj. Can you add a test case that covers it and then I'm super down to merge?

[oyeanuj]

@ianstormtaylor Thanks! Added a test, let me know if it looks good?

[ianstormtaylor]

Hey @oyeanuj looks like a good start. I think we need to edit the test though to handle the more complex case.

Right now, since the text node being inserted into is the first and only node, the offset won't change when the new key is computed. But we should change the test case to be a block with text, inline, text and inserting into the last text node. Then i think we'll end up with a failing test case, since the offset will need to be recomputed to be relative to the new key. (Which you should be able to do adjust using the return value from block.getOffset(textKey) I think.

Let me know how that goes!

[oyeanuj]

@ianstormtaylor I added a set up for a test case like you suggested. Can you take a look to make sure it (block-complex) reflects what you were thinking? If the test case looks good, I'll do the rest of testing it and fixing the failing case.

[ianstormtaylor]

@oyeanuj Looks good!

I think we should also change the tests to not use insertBlock. Instead, they can use document.getBlocks().first() or similar to get the block they need, and insert text into it. That way they are doing less.

[oyeanuj]

Hey Ian, so I added the test case that we discussed and it didn't fail. So clearly I misunderstood your comment above - can you take a look and tell me what were you expecting instead?

[ianstormtaylor]

Hey @oyeanuj I'm not sure what I was talking about.

I'm sorry for the slowness on this. Since this has been opened, it seems like we've had other issues where using offsets isn't the most robust, so I think I'd like to wait to merge this until we get those figured out. Right now we'd been trying to remove all offset-use-across-depths from the codebase I believe, so I'd rather not reintroduce one yet.

[oyeanuj]

@ianstormtaylor That makes sense, happy to wait!

[ianstormtaylor]

As of #3093 (which was just merged), I believe this issue is no longer applicable, because a lot has changed. I'm going through and closing out any potential issues that are not out of date with the overhaul. Thanks for understanding.