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: sync state on input, signal undo levelling #2930

Closed
wants to merge 1 commit into from

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Oct 9, 2017

Description

See #2758.

This PR attempts to let Editable continuously sync state on the input event, and signal for the creation of undo levels when it is triggered by TinyMCE. This keeps the logic for creating undo levels in TinyMCE in use, though not the data.

This is very much a work in progress, but I'm creating a PR now for further discussion.

@aduth would love to hear your general thoughts on this.

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Oct 9, 2017
@gziolo gziolo self-requested a review October 9, 2017 13:00
@gziolo
Copy link
Member

gziolo commented Oct 10, 2017

@iseulde does it also try to solve #2758 (Editable: Invoke onChange on every input), which we need to address before we can merge #2863 (Keyboard Shortcuts: Bind Cmd/Ctrl+S as save).

Some context about what's blocking #2758:

This is a nice usability improvement, but I think I'd want to wait until after we resolve editor content not being kept in sync before moving to merge this, since as you note it can be confusing that Save is not available until moving focus.

@ellatrix
Copy link
Member Author

Yep, sorry, forgot to link #2758.

@aduth
Copy link
Member

aduth commented Oct 11, 2017

Resetting seems smoother, but more complex I guess.

Yeah, I would very much prefer we not have the additional settings argument passed to onChange, just the next value, or at least obscuring this from the concern of the block implementer. I'm having a hard time following the need for this, and will plan to circle back around with a fresher mind tomorrow.

@gziolo gziolo removed their request for review January 27, 2018 18:49
@ellatrix
Copy link
Member Author

ellatrix commented Feb 8, 2018

Closing in favour of #4956.

@ellatrix ellatrix closed this Feb 8, 2018
@ellatrix ellatrix deleted the try/better-undo-stack branch February 8, 2018 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants