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

Editable: Invoke onChange on every input #2758

Closed
aduth opened this issue Sep 20, 2017 · 5 comments
Closed

Editable: Invoke onChange on every input #2758

aduth opened this issue Sep 20, 2017 · 5 comments
Assignees
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@aduth
Copy link
Member

aduth commented Sep 20, 2017

Related: #2750
Explored in #2418
Supersedes #1240

In an effort to keep post state in sync and dramatically simplify handling of content, we should invoke the Editable onChange callback on every input event which occurs within the editable field. Previously we had decided to trigger the callback only when focus leaves the field, resulting in unexpected behavior as described at #1240. The reason for this was performance concerns in serializing content on every keypress with the overhead of our value getter mapping. Since Editable content fields are used in a more granular fashion than normal for a TinyMCE instance (per paragraph, not per document), this is unlikely to pose as much of a concern, and may be helped with efforts of #2750.

The current approach in #2418 implements a throttled callback. While this could remedy performance concerns, it also introduces a new set of concerns with canceling pending callbacks in response to future user actions (save, etc). See Automattic/wp-calypso#17819 as an example of where we have encountered this recently in the Calypso editor where we use this approach.

It will be important to consider how Undo / Redo operates with this approach, particularly with behaviors implemented as part of #1943 with the TinyMCE -> Redux undo handoff. For example, it would be non-ideal for Undo to pop one character at a time when used in succession.

@aduth aduth added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Task Issues or PRs that have been broken down into an individual action to take labels Sep 20, 2017
@ellatrix
Copy link
Member

Apologies in advance if I'm not following well. So would you propose passing changes all the time to the reducers and let them decide how to create undo levels? So TinyMCE will send the content on every keypress, and the undo reducer creates levels based on some criteria? That's what I had in mind for a while, as it would be nice to unify everything under one undo manager. We can then also disable the TinyMCE one.

@aduth
Copy link
Member Author

aduth commented Sep 25, 2017

Yes, it would ideally result in a single undo history. Not clear yet how precisely this would work: If TinyMCE has an event for this, we could merely send this up through to the reducer as a signal (i.e. start recording changes into the same undo entry, split at specific intervals). Also not sure this would specifically be handled by reducers or as an effect (separate action).

@ellatrix
Copy link
Member

Ah, yeah, we could normally send the content, say { content: '...' }, and when there's a new level { content: '...', addUndoLevel: true }. Would be happy to attempt this.

@aduth
Copy link
Member Author

aduth commented Sep 25, 2017

Would be happy to attempt this.

Sure thing. Tagged you as assignee.

@youknowriad
Copy link
Contributor

closed by #4955

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

3 participants