-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Implement #1452 #5236
Implement #1452 #5236
Conversation
Honestly I'm disappointed this hasn't been done sooner, and that the undoManager has had empty toJSON and fromJSON methods for this long. |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5236 +/- ##
==========================================
+ Coverage 87.41% 87.43% +0.01%
==========================================
Files 574 574
Lines 45613 45652 +39
Branches 6935 6935
==========================================
+ Hits 39873 39914 +41
+ Misses 5740 5738 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Hey! Thank you for the contribution! Empty toJSON
and fromJSON
methods are definitely not useful.
The changes look good to me; could you add some basic tests that ensure the output is as expected?
src/undomanager.js
Outdated
toJSON() { | ||
|
||
return { | ||
$redoStack: this.$redoStack, | ||
$undoStack: this.$undoStack | ||
}; | ||
} | ||
|
||
fromJSON() { | ||
|
||
fromJSON(json) { | ||
this.reset(); | ||
this.$undoStack = json.$undoStack; | ||
this.$redoStack = json.$redoStack; | ||
} |
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.
Could you introduce testing for the two methods?
Close |
This affects undomanager not edit session. Don't close. |
I'll add tests as soon as I have a chance. |
Small little update about the usage of these changes, if you want to save state to localStorage or upload to remote server.
That is to say, the toJSON method actually returns a "safe" instance object, not a string. Just let me know if you'd prefer a string and I'll happily make the change. |
@andredcoliveira This is ready to merge now, unless you'd rather me convert the methods to return/accept a string instead of an object. |
Thank you for the contribution! Returning object is fine, it is consistent with existing |
Issue #, if available: #1452
Description of changes: Implements the toJSON and fromJSON methods to allow saving and restoring undoManager's state.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.