-
-
Notifications
You must be signed in to change notification settings - Fork 827
Conversation
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.
I haven't verified that the tests test the behaviour we want, but they do seem to do the right thing and you're probably the person who knows best anyways.
import {diffDeletion} from "../../src/editor/diff"; | ||
|
||
describe('editor/diff', function() { | ||
describe('diffDeletion', 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.
might be good to have tests for multiple characters as well, just in case we've over-engineered it to work with single character diffs
src/editor/diff.js
Outdated
* @param {String} oldValue the previous value | ||
* @param {String} newValue the new value | ||
* @param {Number} caretPosition the position of the caret after `newValue` was applied. | ||
* @return {object} |
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.
this should have a description
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.
lgtm, I think. Still haven't checked the test cases themselves, just that they look like code.
* @return {object} | ||
* @return {object} an object with `at` as the offset where characters were removed and/or added, | ||
* `added` with the added string (if any), and | ||
* `removed` with the removed string (if any) |
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.
we seem inconsistent in our jsdoc: sometimes we put newlines directly against the *
but other times we indent like this. I'm indifferent, but just something I think we should consider in the future.
For element-hq/element-web#9868
Added 69 tests