Plaintext markdown chatinput

[mxstbr]

Status

• WIP
• Ready for review
• Needs testing

Deploy after merge (delete what needn't be deployed)

• api
• hyperion (frontend)

Testing whether switching to a plaintext editing experience could be viable.

[mxstbr]

I've rewritten the chat input from scratch with the minimal amount of functionality necessary (and with hooks because why not!). The only thing that works is sending messages with markdown in threads.

And… it seems to be fine and doable! 👍 I estimate we can get the plaintext chat input with full functionality to a shippable state in 1-2 days. The thread editor would take a bit longer, I think we could get that ready to go in a week.

Pros

• Consistent browser support, including Android
• Less buggy
• Less complexity

Cons

• No WYSIWYG preview (although we could add a preview tab to the thread editor a la GitHub)
• Strange sending UX: first shows raw text from optimistic response, then jumps to formatted once converted on the backend. Might be able to work around this.

I think we should do this given how many complaints we get about the text writing experience. What do you think @brianlovin?

[mxstbr]

Note that the meat of the change right now is in chatInput/index.js (https://github.com/withspectrum/spectrum/pull/4472/files#diff-f37f8727e3ee11c56ff3a6ae6894c358) but the diff is basically useless since I rewrote everything—it's probably easier to just look at the entire file without the diff!

[mxstbr]

Had to upgrade to get compatibility with react hooks

[mxstbr]

Had to upgrade to get react hooks

[mxstbr]

I'm going to go ahead and try to get some more functionality in, let's see how this scales.

[mxstbr]

Getting there, latest commits add quoted message and media message support.

Todos before this is shippable

• Local storage persistence
• Photo size errors
• Switch to same markdown conversion library on both front- and backend
• Make sure everything works when creating DM threads
• Make sure everything works in DM threads
• Make sure everything works in threads
• Fix code blocks
• Fix flow
• Fix e2e tests
• TBD

[brianlovin]

Strange sending UX: first shows raw text from optimistic response, then jumps to formatted once converted on the backend. Might be able to work around this.

We should just transform the plaintext to draftjs in the optimistic response ;)

I think we should do this given how many complaints we get about the text writing experience. What do you think @brianlovin?

You know I'm all about doing this - let's go for it! Will jump in today and see if I can help.

[mxstbr]

We should just transform the plaintext to draftjs in the optimistic response ;)

I know but this isn't as easy as it sounds without shipping a lot more code on the frontend, which I'd like to avoid.

[brianlovin]

I know but this isn't as easy as it sounds without shipping a lot more code on the frontend, which I'd like to avoid.

We could render the optimistic response at ~70% opacity to indicate it's in a processing state?

[mxstbr]

Yeah I think that'd be a good enough solution for now!

[mxstbr]

Latest commit makes optimistic responses 0.7 opacity, looks like this:

Is that acceptable from a UX perspective? Still a lot of jumping around in this specific case, code blocks are definitely a worst-case scenario for this 😕

[brianlovin]

How much does it add to the bundle to transform the plaintext to draftjs in the optimistic response? This outcome feels a bit janky especially on slow networks. This direction feels super good though as far as simplicity of the input!! Brian

…

________________________________ From: Max Stoiber <notifications@github.com> Sent: Sunday, December 16, 2018 11:56 PM To: withspectrum/spectrum Cc: Brian Lovin; Mention Subject: Re: [withspectrum/spectrum] Spike plaintext chatinput (#4472) Latest commit makes optimistic responses 0.7 opacity, looks like this: [optimistic-responses]<https://user-images.githubusercontent.com/7525670/50073621-73922780-01d9-11e9-9fda-b417bc57565d.gif> Is that acceptable from a UX perspective? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#4472 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB1YvDeAwTqyUXFyaJdREUfDj77j4F-Mks5u504RgaJpZM4ZTOMZ>.

[mxstbr]

The one we use on the backend is 32.5kB—that's a lot. 😕 The underlying issue is that we'd have to ship a markdown parser to the frontend, which is never cheap, even though we don't really even need all of markdown for messages. I'll see if I can find something or whip something up that'll only handle inline styles and code blocks…

[brianlovin]

🙏🙏 Brian

…

________________________________ From: Max Stoiber <notifications@github.com> Sent: Monday, December 17, 2018 12:04:12 AM To: withspectrum/spectrum Cc: Brian Lovin; Mention Subject: Re: [withspectrum/spectrum] Spike plaintext chatinput (#4472) The one we use on the backend is 32.5kB<https://bundlephobia.com/result?p=markdown-draft-js@1.3.1>—that's a lot. 😕 The underlying issue is that we'd have to ship a markdown parser to the frontend, which is never cheap, even though we don't really even need all of markdown for messages. I'll see if I can find something or whip something up that'll only handle inline styles and code blocks… — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#4472 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB1YvImJH66_EHwUVouWK1G_ZYmAkAh8ks5u50_8gaJpZM4ZTOMZ>.

[mxstbr]

Found the draft-js-import-markdown library which is "only" ~8kB min+gzip and does everything we need: https://bundlephobia.com/result?p=draft-js-import-markdown@1.2.1

Latest commit uses that to richly render optimistic responses on the frontend, works great as far as I can tell! Maybe we should switch to using that one on the backend too for compatibility 🤔

[brianlovin]

Latest commit uses that to richly render optimistic responses on the frontend, works great as far as I can tell! Maybe we should switch to using that one on the backend too for compatibility

Not sure, I'll let you make that call. 8kb sucks, but I know we've been meaning to do some bundle cleanup anyways at some point, I'm sure we're shipping something dumb that we could cut to neutralize the 8kb :P

[mxstbr]

Geeeez look at this @brianlovin: 2e9d461

Basically, we need almost no code to do the local storage persistence since 1. nothing can crash anymore now that we only store plain text and 2. React hooks are epic. This also works better than before since it keys by the thread ID in localStorage, meaning if you switch between threads it'll show the cached/uncached content! 😍

The only thing I removed was the expiry of cached content—since it's now keyed by the thread there's really no reason to expire anymore.

[mxstbr]

There is a bug where you can't create a new DM thread with both an image and some text in the message, but that bug is generally around so I won't fix it since it'd require a bit of a backend refactor which I'd like to avoid.

I'll log it in a new issue.

[mxstbr]

I honestly don't really know how to solve single line break paragraphs. @sstur how hard would it be to get support for that into draft-js-import/export-markdown?

[brianlovin]

EDIT: After looking at the recording, you just need to update the dependencies in the API directory! The draft-js-import-markdown lib on the frontend is correctly converting the code block on the frontend, but you're running the old version in the API, which is why it disappears!

Whoops! Thanks for the extra details :)

I honestly don't really know how to solve single line break paragraphs.

Only short-term workaround I could think of would be to convert from a 'chat input' style input to a 'comment textarea' style; if it looks like a comment box where 'enter' doesn't send the message by default, we could better explain that it behaves the same way as any other markdown document.

[sstur]

@brianlovin Those screencaps are super helpful!

@mxstbr
Re: "Markdown only parses double line breaks as new paragraphs, so it's collapsing single line breaks"

I do believe that is the spec-compliant behavior for Markdown (I use the term "spec" loosely ; )

However I do believe the "Github Flavored Markdown" variant changes that behavior. It considers a single line break as a <br> and a double line-break as a new <p>. (You can inspect this element to see, notice before the "Re" above there's a <br>)

I'd be happy to support both behaviors, in fact, it might already be an option baked in to the underlying markdown parser we use.

Edit: I forget we forked that library instead of importing it, can't remember the reason off the top of my head.

Edit 2: The parsing library does indeed support github flavored markdown including the linebreaks feature however draft-js-import-markdown is not passing those options through. I made a PR if you want to take a look.

[mxstbr]

Latest commit makes single line breaks work, so this is ready for another code review @brianlovin!! 🎉

Thanks @sstur for the quick help, appreciated

[brianlovin]

Gah, @mxstbr this is SO close!!! Sorry I just found this one: editing a code block doesn't transfer the code block into the edit message input:

https://www.dropbox.com/s/q2iv1z7bb205uv5/Kapture%202019-01-10%20at%2018.24.24.gif?dl=0

[mxstbr]

It does! Code blocks in markdown can be written with four spaces in front of the code (like on StackOverflow), only GitHub-flavoured markdown added support for fenced code blocks—so what you're seeing there is a code block.

@sstur how hard would it be to add a GFM mode to draft-js-export-markdown?

[sstur]

@mxstbr, I think this is not too hard. Probably something like this:
sstur/draft-js-utils#159
(please take a quick moment to sanity check my work)

Such an option will solve your immediate problem, but I'd venture to say there's a larger underlying issue which I'd describe as follows:

Many different markdown representations can produce a single visual output (such as extraneous newlines, different formats for (a) italic (b) italic and (a) bold (b) bold (need to view this source to see the difference there) as well as code blocks and probably more. Same is true for many markup formats such as HTML where <b><i>Hi</i></b> is the same visual representation as <i><b>Hi</b></i>.

If I am understanding your implementation, that information is lost in a round trip to DraftJS model and back to markdown.

Maybe this is not a big deal, if your intent is to normalize the user's input into some well-formed canonical representation (like what Prettier does for my code). But, this may seem quirky to the user in the context of composing messages. You might consider doing what Github does, which is store the original text the user typed, either in addition to or in place of the parsed model.

For Github, you can see that by typing some text in a comment here, then going back to edit it. Your original text will always populate the editor, even if you forgot to close a code block. I believe it's the same in chat programs (can verify with Hangouts Chat, although defo don't base your design choices on that product 😂).

Thoughts?

[mxstbr]

I generally 100% agree and I'd love to do it that way, but in our situation right now it's not feasible to spend that much time on engineering a perfect editing experience—it just has to be good enough™️!

[sstur]

The gfm: true option has landed in draft-js-export-markdown@1.2.2. Happy Spectruming!

[mxstbr]

Perfect, thank you so much @sstur! Latest commit upgrades to that version 💪

@brianlovin this is ready for another review. Going to branch off of this and start work on the plaintext editor!

[brianlovin]

@sstur you are the best, thank you for all your help here! @mxstbr this feels like we're as close as we'll get until we get more people using this in real situations! Let's ship!

[mxstbr]

Deploying to alpha!

[mxstbr]

Deployed to alpha, feels pretty good 😎 I think we should alias this to prod!

[mxstbr]

This is live in production, and I've also posted release notes! 🎉 https://spectrum.chat/spectrum/general/spectrum-v2-6-0-a-brand-new-chat-input~84352ff4-9feb-4cce-ab05-575617ad5176

[brianlovin]

@mxstbr I saw a handful of logs come in that indicate we might not be properly handling the backend supporting old draftjs messages being sent. Did you see these?