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

Spike Replacing CodeMirror with Draft-js #93

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

stevemolitor
Copy link
Contributor

@stevemolitor stevemolitor commented Dec 15, 2017

Spike using Draft-js for pretty-text-input, replacing CodeMirror.

Very hacky right now. Doesn't propagate changes thru yet.

One issue is how to propagate changes efficiently. The Draft-js Editor component is a controlled component, that passes an EditorState object representing the current state of the editor, as a structured object. You can serialize between structured EditorState and say plain text with curlies, but that is expensive. Draft-js recommends putting EditorState directly in (for example) your redux store, and only serializing when persisting. We don't want to serialize back to plain text every key stroke. At a minimum we'll want to debounce. Or maybe just call onChange with the serialized value on things like blur, tag selection.

draftjsdemo

Spike using Draft-js for pretty-text-input, replacing CodeMirror.
Very hacky right now. Doesn't propagate changes thru yet.
@jdeal
Copy link
Contributor

jdeal commented Dec 15, 2017

What about copy-paste? Is there a solution for when you select and copy text to get the value (and not the rendered label)? I can think of some goofy solutions, so I guess I should say: is there a relatively straightforward solution?

@jdeal
Copy link
Contributor

jdeal commented Dec 15, 2017

We debounce already with CodeMirror, so is there any difference between what we do now and what we'd do with Draft.js?

@stevemolitor
Copy link
Contributor Author

stevemolitor commented Dec 15, 2017

What about copy-paste? Is there a solution for when you select and copy text to get the value (and not the rendered label)? I can think of some goofy solutions, so I guess I should say: is there a relatively straightforward solution?

That's on my todo list! Nothing super straightforward that I'm aware of yet. At this point my solution would be to implement handlePastedText, and if it's a curly, parse the tag and call handleChoiceSelection(tag).

Not too bad, but what about manually typing in {{firstName}}? With the current CM impl, as soon as you type the last }, it will morph into a pretty-tag. Not aware of a straightforward way to do that yet.

With a 'decorator', you can write a function to tell Draft how to match sections of text and render them with a custom component. Draft calls that function for you. Perfect. However Draft will not treat text inside of those atomically, ie you can delete single characters instead of the whole pill. So you use 'entities' instead - but I wasn't sure how to combine that with those simple text-matching decorators. May be just lack of knowledge on my part at this point.

@stevemolitor
Copy link
Contributor Author

stevemolitor commented Dec 15, 2017

We debounce already with CodeMirror, so is there any difference between what we do now and what we'd do with Draft.js?

I guess I was worried about the round-tripping. Formatic is a controlled component as you know, and I worry about convert to text to pass to onChange, and then converting from text back to EditorState every key press, even with debouncing.

@stevemolitor
Copy link
Contributor Author

Update to my comment above re copy paste: I only addressed pasting, not copying.

For copy, I don't know yet!

@jdeal
Copy link
Contributor

jdeal commented Dec 16, 2017

I guess I was worried about the round-tripping.

But that's the point of debouncing. You only round-trip on the debounce ends. So if serialization/deserialization is no more expensive than CodeMirror, then it's at least no worse than what we currently have.

@stevemolitor
Copy link
Contributor Author

stevemolitor commented Dec 16, 2017

But that's the point of debouncing. You only round-trip on the debounce ends. So if serialization/deserialization is no more expensive than CodeMirror, then it's at least no worse than what we currently have.

My impression is that it could be more expensive. But I'll measure.

Draft-js recommends not debouncing and then triggering re-render here, to avoid interaction problems caused by things getting out of sync between the user's typing and the new state after debounce:

you should separate the delay behavior from your Editor state propagation. That is, you must always allow your EditorState to propagate to your Editor component without delay, and independently perform batched updates that do not affect the state of your Editor component.

(emphasis added)

I'm not sure if CM really 'serializes' in the sense we are talking about here. The value prop maintained by CM is just plain text. Your onChange handler gets passed that value as plain text. Separate from that it maintains metadata about how to apply highlighting, etc to certain ranges of that data. That can be serialized. You can also serialize say the structured undo history. But the value is always just plain text, no serialization required.

With Draft-js, you pass a structured EditorState object as an initial prop or state item. onChange receives an EditorState object. To apply discrete changes (like picking a new tag), you change that immutable EditorState object efficiently via Immutable.js. (You don't use Immutable.js directly; they have helpers. But that's what's happening under the hood.) The idea is to avoid 'round-tripping' between ie plain text, HTML, markdown, or what have you, and the structured content every key stroke. That would defeat the purpose of using Immutable.js and its persistent data structures.

Serializing between EditorState and your native data structure (which could be HTML, markdown, plain text, etc) is up to you, but probably best to do it only when necessary (like saving to the server). And not re-rendering the draft-js editor afterwards, or at least not doing that very often. That throws away and re-creates the whole persistent data structure. Draft-js recommends instead putting the structured EditorState directly in say your redux store, or component state (what I'm doing here).

And that's probably the difference between CodeMirror and Draft-js: CodeMirror is a text editor component; Draft-js is a structured content (aka 'rich text') editor. (That's just my take.)

@stevemolitor
Copy link
Contributor Author

stevemolitor commented Dec 16, 2017

Re copy, this is discouraging: facebookarchive/draft-js#787

Just an update - I don't expect any support for this in the near future. It would be an interesting long term goal but we have shifted to have internal folks work on some other fixes and features for Draft.

Making copying a pretty-pill from one pretty-text instance to another work might be a small science project. :( Issue above has some ideas using ClipboardData object. :(

(We gotta make that work.)

@jdeal
Copy link
Contributor

jdeal commented Dec 16, 2017

you must always allow your EditorState to propagate to your Editor component without delay

I'm not proposing anything that deviates from that. Internally, the editor would use normal EditorState. But it would debounce a sync of that state outside itself. That's effectively what we do now. Without that debounce, syncing CodeMirror was way too expensive for big fields. That's why I added the debounce. It's weird, but it has worked fine since then. Now, if serializing/deserializing EditorState is too expensive, then that won't work, because it will cause noticeable pauses in the UI. I'm guessing anything 200ms or higher for big fields would be where it would be dangerous. But if it stays at or under 100ms or so, we're fine.

@jdeal
Copy link
Contributor

jdeal commented Dec 16, 2017

Yeah, I've been down the road of hooking into copy-paste, and it wasn't fun. But as usual, I remember IE being the worst offender. But even with decent browsers, yes, it is likely to be a science experiment.

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

Successfully merging this pull request may close these issues.

2 participants