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

Delete behavior is off #24

Open
adjourn opened this issue Mar 13, 2019 · 6 comments
Open

Delete behavior is off #24

adjourn opened this issue Mar 13, 2019 · 6 comments
Labels
bug confirmed bug slate-lists

Comments

@adjourn
Copy link

adjourn commented Mar 13, 2019

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:

  • If current and next item have content, next item is deleted and its content is appended to current's
  • If current has content and next is empty, next list item is just deleted
  • If current is last item in list and has content, nothing happens after Delete

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

  • If two empty list items are next to each other, pressing Delete moves caret to other empty item, this behavior repeats if Delete is pressed again, nothing gets removed
  • If current is last and empty item in list, caret is moved to previous list item, last empty item only gets removed when Delete is pressed again (when caret is in previous item)
  • If current is first and empty item in list, caret is moved off the list (previous block) and I get two list bullets side-by-side situation, few frames later, I get the following error:

err

These descriptions might get pretty cryptic, my apologies.

@adjourn
Copy link
Author

adjourn commented Mar 13, 2019

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
Copy link
Collaborator

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

@adjourn
Copy link
Author

adjourn commented Mar 21, 2019

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
Copy link
Collaborator

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
Copy link
Author

adjourn commented Mar 24, 2019

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug confirmed bug slate-lists
Projects
None yet
Development

No branches or pull requests

2 participants