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

Replace Draft with Slate #1890

Merged
merged 108 commits into from Jul 16, 2018

Conversation

Projects
None yet
7 participants
@ara4n
Copy link
Member

ara4n commented May 12, 2018

This PR replaces Draft with Slate, with the intention of fixing pretty much all of the RTE bugs we currently have available and making the composer feel much more snappy, polished and robust. The strategy taken is to 1) comment out everything Draft specific; 2) gradually go through porting each commented-out section to Slate, whose API is luckily relatively similar (albeit much simpler). Some things to note:

  • I've deliberately avoided any significant refactoring in order try to make things easier to compare with the original during review. However, MessageComposerInput is screaming to be split up into multiple classes (and for its massive methods to be split up to be more manageable).
  • Slate refers to its editor state as a 'Value'. This feels like a really ambiguous name, so I've referred to it throughout as editorState instead - which is conveniently what Draft used already.
  • Draft has the confusing concepts of both editorState and contentState. The latter has been unified with editorState.

Current progress is:

  • Make history work (storing history as Slate values in local storage)
  • Author normal messages with Slate
  • Author MD messages with Slate
  • Make autocompletes work (rooms, users)
  • Make emoji completions work
  • Make emoji-one work
  • Wire up formatting buttons
  • Wire up keyboard shortcuts
  • Debug RT authoring
  • Paste images is broken
  • Focus is lost after all slash commands (e.g. /nick, or /tint)
  • Hide highlight buttons when in MD mode somehow
  • Make buttons highlight correctly when in RTE
  • Fix quoting
  • Remember where the cursor is in the composer when switching back and forth between rooms
  • Serialising MD when quoting chokes stupidly on blockquotes with nested paras
  • editor doesn't wrap on massive unbreakable lines any more
  • Inserting ™ via MacOS emoji console doesn't work (rather than emoji autocomplete)
  • autocomplete emoji leaks non-emoji before emojione kicks in
  • ctrl-backspace doesn’t work on Windows (but alt-backspace works on macOS)
  • undo is broken.
  • round-tripping code blocks is a bit wobbly (as tested on the slate doc)

Nice to have:

  • autocompleting plain text emoji consumes space afterwards
  • copy-paste mentions from & to RTE doesn't work
  • Make copy-pasting chunks of timeline into the RTE do the right thing
  • linkify only pillifies aliases it already knows about
  • Make RT authoring work (although I wonder whether anyone actually particularly likes or uses it. It's not that hard to have, but is it worth it?)
  • Insert an undo state every time you start a new word.
  • Make HTML pasting work (RT authoring is probably worth it to support this, though)
  • emoji in pills aren't emojioneified in timeline

Optional:

  • Support round-tripping between MD & RT. I'm not convinced this is ever going to work very well, and it might be easier just to throw away the input when you switch between modes than have a flakey experience. That said, the code is already there, so should probably evaluate at the point of either deleting it or restoring it.
  • Allow the RTE to be entirely disabled? For simpler accessibility and to help folks who were swapping out the textarea for vim etc.
  • Make EmojiOne optional

ara4n added some commits May 13, 2018

autocomplete polishing
* suppress autocomplete when navigating through history
* only search for slashcommands if in the first block of the editor
* handle suffix returns from providers correctly
* fix SelectionRange typing in the providers
* fix bugs when pressing ctrl-a, typing and then tab to complete a replacement by collapsing selection to anchor when inserting a completion in the editor
* fix vector-im/riot-web#4762
autocomplete polishing
* suppress autocomplete when navigating through history
* only search for slashcommands if in the first block of the editor
* handle suffix returns from providers correctly
* fix bugs when pressing ctrl-a, typing and then tab to complete a replacement by collapsing selection to anchor when inserting a completion in the editor
remove // support, as it never worked
if you want to escape a /, do it with \/ or just precede with a space
unify setState() and onChange()
also make emoji autocomplete work again
also remove the onInputContentChanged prop
also slateify the onInputStateChanged prop

This was referenced Jul 16, 2018

@t3chguy

This comment has been minimized.

Copy link
Collaborator

t3chguy commented Jul 17, 2018

copy-paste mentions from & to RTE doesn't work

works for me:
m2

@t3chguy t3chguy referenced this pull request Jul 17, 2018

Merged

Moar Slate Fixes #2069

@makedir

This comment has been minimized.

Copy link

makedir commented Nov 2, 2018

Does anyone even test their changes here!? New bug:

  1. writing text
  2. write emoticon
  3. you cant write on, space is disabled after the emoticon
@MTRNord

This comment has been minimized.

Copy link
Contributor

MTRNord commented Nov 2, 2018

@makedir i think a full New issue would make more sense and mentioning the pr than writing that into one of the prs. (as afaik this change happend with more than 1 pr).

Also as far as I remember I can write after emoji just fine (Windows 10 and Linux Arch)

@makedir

This comment has been minimized.

Copy link

makedir commented Nov 2, 2018

No I cant with Windows 10 client latest version (riot.im), example:

"This is a test <3"

If I press space after <3 nothing happens (cursor doesnt move), I cant write on and have to press enter.

@MTRNord

This comment has been minimized.

Copy link
Contributor

MTRNord commented Nov 2, 2018

@makedir thats not a valid emoji in riot. Valid emoji would be something like: :innocent:. Tab on the heart plain emoji causes that you leave the composer

@makedir

This comment has been minimized.

Copy link

makedir commented Nov 2, 2018

And? Than thats even two bugs. If I type <3 your client opens the emoticon box and wants to replace it with a heart, same as :-). If the box is open space doesnt work it seems.

@tulir

This comment has been minimized.

Copy link
Contributor

tulir commented Nov 2, 2018

  1. Don't post bug reports in comments. Issues exists for that.
  2. Sounds like you found this bug that was already reported and fixed: vector-im/riot-web#7509
@t3chguy

This comment has been minimized.

Copy link
Collaborator

t3chguy commented Nov 2, 2018

Also this was tested hundreds of times. Updates to the slate library since then likely introduced new issues.

@matrix-org matrix-org locked and limited conversation to collaborators Nov 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.