search

bustle / mobiledoc-kit

A toolkit for building WYSIWYG editors with Mobiledoc
https://bustle.github.io/mobiledoc-kit/demo/
MIT License
1.55k stars 150 forks source link

[Bug Fix] don't modify tail.section if offset is 0 (aka tail.section is not selected) #713

Closed nikse closed 4 years ago

nikse commented 4 years ago

Fixes 2 bugs around double clicking a paragraph to modify/copy-paste it:

  1. Issue: if a user double clicks to select and copy a paragraph that has a card following it, the card gets caught up in the copy and will also be pasted wherever you try to paste the paragraph Fix: rather than copying the card we now insert a new blank paragraph

  2. Issue: if a user double clicks to select a paragraph to add section level markup and there is another paragraph following the selected one the second paragraph will also have the section level markup applied to it Fix: only apply markdown to selected sections

ZeeJab commented 4 years ago

@nikse Made a small change to your PR. How does this look?

  1. I don't think hardcoding special behavior for ul is necessary. Why does the special behavior apply to ul but not ol? It looks like this was here just to satisfy some tests that happened to use ul. Is there a good reason for this special casing?
  2. Rather than pushing logic for the special case throughout the toggleMarkup function, in this commit we can normalize the range object at the beginning of the method to only include the sections we want to be modified.
nikse commented 4 years ago

  1. I don't think hardcoding special behavior for ul is necessary. Why does the special behavior apply to ul but not ol? It looks like this was here just to satisfy some tests that happened to use ul. Is there a good reason for this special casing?

    • I think this was because ul's were already acting correctly, i.e. adding a blank bullet at the end of the list in this situation

  2. Rather than pushing logic for the special case throughout the toggleMarkup function, in this commit we can normalize the range object at the beginning of the method to only include the sections we want to be modified.

    • I think this sounds good, I'll test it out again (I'm a bit rusty on what I did here)
ZeeJab commented 4 years ago

@nikse why would uls behave differently than ols? what were the steps to reproduce so I can test out? I just want to make sure there's not something I'm missing. Otherwise the changes I made to this branch should be good and we can merge 👍