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

Insert outside inline when selection is at the end #3260

Merged

Conversation

dmarkow
Copy link
Contributor

@dmarkow dmarkow commented Dec 6, 2019

Is this adding or improving a feature or fixing a bug?

Fixing a bug

What's the new behavior?

screencast 2019-12-06 07-46-47

When inserting text at the end of an inline (e.g. a link) using a selection as the insertion point, the text will be added outside the inline. Passing an explicit point/path to Editor.insertText will retain the old behavior of inserting inside the inline.

Input:

<editor>
  <block>
    one
    <inline>
      two
      <cursor />
    </inline>
    three
  </block>
</editor>

Output from typing "four" or using Editor.insertText(editor, "four") (with no explicit at point/path)

<editor>
  <block>
    one
    <inline>
      two
    </inline>
    four
    <cursor />
    three
  </block>
</editor>

Output from calling Editor.insertText(editor, "four", { at: { path: [0, 1, 0], offset: 3 } }):

<editor>
  <block>
    one
    <inline>
      twofour
    </inline>
    three
  </block>
</editor>

How does this change work?

I updated the insertText function to check if the cursor is at the end of an inline, and if so, it moves the selection outside of the inline before adding the text.

I have no idea if this is the right approach, so any feedback is greatly appreciated.

Have you checked that...?

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)

Does this fix any issues or need any specific reviewers?

See #3151 - this addresses the inline portion of that issue.

@ianstormtaylor
Copy link
Owner

Hey @dmarkow thank you for taking the time to investigate this!

I think we actually want to define this in the insert_text command, instead of changing the behavior of Editor.insertText. This way people can override insert_text and change the behavior (eg. for "sticky inlines" which has been request by a decent amount of people). And that way the Editor.insertText stays as predictable as possible for when using it in other situations.

@dmarkow dmarkow force-pushed the normalize-insert-text-inlines branch from fe9084e to 289558a Compare December 7, 2019 02:17
@dmarkow
Copy link
Contributor Author

dmarkow commented Dec 7, 2019

Just made another pass on this. I moved the logic to insert_text, and updated it to only apply when the range is collapsed. Dealing with the expanded range is tricky since Editor.insertText handles some stuff with voids/deletion, and it seems like the issue wasn't affecting expanded ranges in the same way anyway.

As you mentioned, overriding insert_text to not do the inline edge check would be very easy now (it would go back to being a single Editor.insertText command)

I did catch another bug while working on this.

screencast 2019-12-06 20-26-02

If you highlight across the edge of an inline and type, the first letter you type becomes highlighted/selected, then the second letter you type replaces it. So if you quickly type one after selection across the edge, the o will be highlighted and then overwritten by n, with resulting text of ne. It looks like this was because the anchor/focus point of the selection were on difference edges of the inline. I updated it to set the selection anchor and focus to at.

Let me know how this looks. If it's good to merge, I can clean up the commit history on it first.

@ianstormtaylor
Copy link
Owner

Looks great! Thanks for the detailed explanation too, that made it very easy to review. No need to cleanup the commits either because I just use GitHub's "squash and merge" strategy so it all goes into a single commit.

Thank you @dmarkow!

@ianstormtaylor ianstormtaylor merged commit b426864 into ianstormtaylor:master Dec 7, 2019
z2devil pushed a commit to z2devil/slate that referenced this pull request Dec 6, 2024
* Insert outside inline when selection is at the end

* Move inline edge check to insert_text

* Fix selection point after deleting text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants