Updated undo and redo to accept a number of steps#32
Conversation
|
Hey @ivoilic, thanks for the PR! I love this idea and will review it formally later today. Thanks! |
charkour
left a comment
charkour
left a comment
There was a problem hiding this comment.
This works great! Thanks for the PR.
As for the breaking change, do you think it would be good to release v2 of zundo? Or is it small enough to keep it in v1? I'm leaning towards a v2.
|
v2 Makes sense. In that case any other potentially breaking changes you want to roll in at the same time? I can add the fix for #24 clear -> clearUndoHistory into this PR for example. |
|
Sure, that is fine with me! Thank you. |
|
Unless you can see any other issues or last minute breaking features to add this good to merge once the other PR has been merged and released. 🙏 |
| undo: (steps?: number) => void; | ||
| redo: (steps?: number) => void; |
There was a problem hiding this comment.
Now that I'm looking at this, could the parameter of undo and redo be something like options that is an object? Right now the object would be options: { steps?: number }, but I could see us expanding the options. That way we wouldn't have breaking changes in the future if we want to add more. What do you think?
There was a problem hiding this comment.
Not sure what other options it would make sense to add to these functions. If you can think of a few important ones than sure, otherwise in the future you could always add a second parameter that accepts an options object. Not having to pass on options object feels more streamlined to me.
There was a problem hiding this comment.
True, can't think of any off-hand. I'm good with keeping this streamlined until there is actually a use for multiple parameters.
|
Once you solve the conflicts, I can merge this into the v2 branch and release a beta package. I am re-writing some of the package to have better typescript support in v2, and have changed some of the APIs slightly. I can tag you once I make a PR with the new API to get your feedback on it. Your awesome logo will be in the v2 readme (along with a link to your twitter or whatever profile you would like). |
ivoilic
force-pushed
the
feature/undo-steps
branch
from
176ddea to
ea0375a
Compare
charkour
left a comment
charkour
left a comment
There was a problem hiding this comment.
Looks great! I'll release a beta package since there are a couple other changes I'd like in v2.
Really appreciate the work!
|
The beta has been published. Please let me know if there are any issues! |
2 participants
Summary:
This PR adds a new optional parameter to the undo and redo function which specifies a number of steps to undo/redo. This parameter is optional and defaults to 1 as you might expect. The same as with a single step undo/redo, a multistep undo/redo will on undo/redo as many steps as possible.
Why?:
Being able to change state back/forward by more than one step will allow for more complex interactions with the history.
Breaking Changes:
Unfortunately this update causes an issue when undo/redo are passed directly to
onClickas in the previous documentation. By default the event is passed which throws a type error since the functions are now expecting a number as the first parameter.Tests:
I've added some basic tests but further more detailed tests should be implemented eventually.