Kit / slate-plugins

A collection of plugins for SlateJS, maintained by ConvertKit
https://convertkit-slate-plugins.netlify.com/
MIT License
52 stars 22 forks source link

Delete behavior is off #24

Open adjourn opened 5 years ago

adjourn commented 5 years ago

By current I mean the list item where caret is located when Delete is pressed.

I should also note that depth doesn't play any role with Delete, next or previous list items are literally visually next and previous, even if different depth (I don't have a problem with that).

Good cases:

Bad cases: Why bad? I personally don't care about Delete functionality at all but it would be nice if it worked properly if it's already there. From list documentation:

Delete or Backspace at the start of an item: remove the item

err

These descriptions might get pretty cryptic, my apologies.

adjourn commented 5 years ago

Looks like Delete key is not handled at all by plugin.

Would it make sense to add delete: onBackspace, or do you have custom behavior in mind?

brendancarney commented 5 years ago

Possibly. I'll check out other editors to see what the behavior is and mimic that.

adjourn commented 5 years ago

Did some testing with native behavior, Slate mimics it successfully but it seems to trip on nested lists.

https://codesandbox.io/s/q9w2v0xlmq

delete basically works like a backward backspace, i.e deletes the character in forward direction. If selection is at the end of line/block (li in this case), delete moves next line/block content to currently selected one (and removes li wrapping due to normalization?). Not sure what the target of delete is in this situation, newline or list item element itself.. Still noob at this subject.

It behaves the same whether currently selected element is empty or not, it shouldn't delete current list item, even if it's empty. If there's no more content after selection (chars nor blocks), delete does nothing.

If selection is not collapsed, delete behaves like a backspace.

Also tested slate-edit-list, it has the same bug.

brendancarney commented 5 years ago

Thanks for the research. That makes sense to me. Delete forward, including bringing the text up from the next list item. Let me know if you want to take a shot implementing it, and if not, I can go ahead and do it. At the very least, we can get the docs updated to remove the current explanation.

adjourn commented 5 years ago

I was about to take a shot but..

yarn
yarn bootstrap
yarn start

Im on Linux at the moment. On yarn start:

Failed to compile.

./packages/slate-code/src/index.js
Module not found: Can't resolve '@convertkit/slate-keymap' in '/home/USER/Documents/git/slate-plugins/packages/slate-code/src'
/home/USER/Documents/git/slate-plugins/node_modules/webpackbar/dist/utils/index.js:58
  return path.replace(cwd, '');
              ^

TypeError: path.replace is not a function
    at shortenPath (/home/USER/Documents/git/slate-plugins/node_modules/webpackbar/dist/utils/index.js:58:15)
    at /home/USER/Documents/git/slate-plugins/node_modules/webpackbar/dist/plugin.js:182:43
    at SyncHook.eval [as call] (eval at create (/home/USER/Documents/git/slate-plugins/node_modules/tapable/lib/HookCodeFactory.js:19:10), <anonymous>:7:1)
    at SyncHook.lazyCompileHook (/home/USER/Documents/git/slate-plugins/node_modules/tapable/lib/Hook.js:154:20)
    at Watchpack.watcher.compiler.watchFileSystem.watch (/home/USER/Documents/git/slate-plugins/node_modules/webpack/lib/Watching.js:139:33)
    at Object.onceWrapper (events.js:285:20)
    at Watchpack.emit (events.js:197:13)
    at Watchpack.EventEmitter.emit (domain.js:481:20)
    at Watchpack._onChange (/home/USER/Documents/git/slate-plugins/node_modules/watchpack/lib/watchpack.js:118:7)
    at Watchpack.<anonymous> (/home/USER/Documents/git/slate-plugins/node_modules/watchpack/lib/watchpack.js:109:8)
    at Watcher.emit (events.js:197:13)
    at Watcher.EventEmitter.emit (domain.js:481:20)
    at /home/USER/Documents/git/slate-plugins/node_modules/watchpack/lib/DirectoryWatcher.js:101:9
    at Array.forEach (<anonymous>)
    at DirectoryWatcher.setFileTime (/home/USER/Documents/git/slate-plugins/node_modules/watchpack/lib/DirectoryWatcher.js:99:42)
    at DirectoryWatcher.onFileAdded (/home/USER/Documents/git/slate-plugins/node_modules/watchpack/lib/DirectoryWatcher.js:250:7)
brendancarney commented 5 years ago

Ah, I didn't see this message earlier. You may need to run:

./node_modules/.bin/lerna link

I used lerna for this but I'm not super experienced with it.