-
-
Notifications
You must be signed in to change notification settings - Fork 146
Add undoCommand & redoCommand for prosemirror-history compatible API
#183
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
Conversation
dmonad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks Nick!
I'm only on the fence about the undoCommand. It seems to be too similar to undo.
| undoManager.redo() | ||
| return true | ||
| } | ||
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea that redoCommand returns true if undo happened?
In that case I suggest to return undoManager.redo() != null, which is true iff the undo manager actually performed a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't neccesarily the undo happened, but that it could happen. So, I think that the current form is okay. You can see more information here: https://github.com/yjs/y-prosemirror/pull/183/files#r2054024808
| * Redo the last user action if there are redo operations available | ||
| * @type {import('prosemirror-state').Command} | ||
| */ | ||
| export const redoCommand = (state, dispatch) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this only undo when dispatch is specified?
This command seems to be quite similar to the undo function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prosemirror commands can be invoked in two ways, with different purposes behind them:
commandFn(state, state.apply)which is exactly as you'd expect, it gives the current state and supplies a function that allows you to dispatch a transaction to transition you to the next state. In this form, the return type ofcommandFnis a boolean which tells you whether the command executed successfully (i.e. met criteria for the command, and was able to dispatch successfully).commandFn(state, undefined)this is a lesser-known form of a command which allows you to check whether a command can run given the current editor state. The returned boolean tells the caller whether the current state meets the criteria of the command, but does not actually attempt to change the editor state at all, it is meant to be side-effect free.
So, the second form is essentially a dry-run for the command.
You can see an explanation of this behavior in the prosemirror reference guide: https://prosemirror.net/docs/guide/#commands
In the context of y-prosemirror, this is useful because it allows for checking whether the editor can undo in the current state. You can imagine disabling a button because there is no undoStack to undo from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
& yea, undoCommand & undo are very similar, but I didn't want to break backwards-compat by forcing the caller to specify the dispatch function from one version to the next
|
@nperez0111 Are you okay with these changes? 96db316 Basically, when dispatch is defined we return whether a change was actually undone. Otherwise, return |
Ah, yea these look great @dmonad! |
|
Okay, will make a release 👍 |
This refactors the
undo-pluginto:undo-pluginstateundoCommandandredoCommandexports which will only modify the state ifdispatchis presentundoCommandandredoCommandnow only undo/redo if there is something to actually undo/redoI made these as separate commands since this was going to be a breaking change for existing users (since
dispatchis now required to execute).Why do it like this though?
This is more in-line with how prosemirror intends things to work. Where it can be useful to have a command run without the dispatch function to see if the command can run (e.g. to enable/disable undo & redo buttons).
This is also matching the implementation set forth by prosemirror-history, making it trivial to replace the two implementations