Update color picker when user makes edits in host editor #2147
Conversation
ghost
assigned 
|
Assigning to @peterflynn |
…lems in some cases.
…esn't cause the color editor to go out of sync.
… and leaves the color in an invalid state.
|
OK, this should be ready for review. Note that in the scenario in #2140, we now collapse the editor. I had originally made it so that we would keep the editor open on the chance that the user might make the color valid again (and correctly update the range in that case), but since we can't really tell whether the user is going to do that in the future, it seems safer to just collapse the editor immediately. We could try to make some smarter UI for this in the future. |
| @@ -38,6 +38,9 @@ define(function (require, exports, module) { | |||
| this.startBookmark = startBookmark; | |||
| this.endBookmark = endBookmark; | |||
| this.setColor = this.setColor.bind(this); | |||
| this.handleHostDocumentChange = this.handleHostDocumentChange.bind(this); | |||
| this.isOwnChange = false; | |||
| this.isHostChange = false; | |||
There was a problem hiding this comment.
I know we don't have an official style policy for this, but I like to see the instance properties documented somewhere. In some of our classes we use dummy prototype copies (assigned null) to hang JSDocs off -- that's one option. (Ditto for the existing color, startBookmark & endBookmark if it's not too much trouble).
There was a problem hiding this comment.
I broke them out, so we can add the JSDocs as part of the JSDoc task.
|
Fixes pushed, ready for re-review. |
| result.onAdded(); | ||
| inline = result; | ||
| }); | ||
| waitsForDone(editorPromise, "open color editor", 500); |
There was a problem hiding this comment.
Nit: the timeout arg is optional in waitsForDone(). Probably no reason not to use the default value here.
|
Done reviewing. |
|
Updates pushed. |
| }); | ||
| }); | ||
|
|
||
| it("should not update the end bookmark to a shorter valid match if the bookmark still exists color becomes invalid", function () { |
There was a problem hiding this comment.
Nit: "if the bookmark still exists and color becomes invalid"
(can fix this in a later pull request's commit if you want)
There was a problem hiding this comment.
Yup, will fix when I finish up the unit tests.
|
Cool, looks good then |
Update color picker when user makes edits in host editor. Track color's text range more robustly. Add some initial unit tests.
In addition to that behavior change, I also factored out logic for getting the current host editor range associated with the inline editor. I also tweaked that logic to make it a little more robust to host edits--if the end bookmark goes stale, instead of just assuming that the relevant range still corresponds to the original color length, we first re-check the color regex to try to determine the end.
Note that this is currently independent of #2142. We might want to reuse the logic from that pull request that avoids synchronizing when the text input field contains an invalid color value, and apply that to changes from the host editor as well. However, this pull could be reviewed independently of that, and we could merge the implementations later; the change would be to make
handleHostEditorChange()call that factored-out logic instead of directly callingcommitColor().