launchpad-reviewers team mailing list archive

[Bayard 'kit' Randel <kit.randel@xxxxxxxxxxxxx>]

Bayard 'kit' Randel has proposed merging lp:~blr/launchpad/inline-diff-comment-keybind into lp:launchpad.

Commit message:
Provides a save shortcut key for the inline diff comment editor, a tooltip, and a relevant yui test.

Requested reviews:
  William Grant (wgrant)

For more details, see:
https://code.launchpad.net/~blr/launchpad/inline-diff-comment-keybind/+merge/243087

Summary
Users have suggested that when providing multiple inline diff comments, it is easy to forget to save an initial comment, before moving on. This is a small quality of life improvement, providing the shortcut Ctrl+Enter in the editor, and a relevant tooltip.

Proposed fix
This does not address the issue directly, but given programmers tend to be very keyboard oriented, providing a shortcut for saving a comment will generally improve workflow and reduce frustrations with the limitations around multiple editor states. Allowing for multiple editors in concurrent editing states should potentially be considered in a later MP.

Implementation details
The editor's keydown event now calls widget.save() on ctrl+enter.

Tests
The test for this feature can be run using the standard yui test runner:
xvfb-run ./bin/test -x -cvv --layer=YUITestLayer

Demo and Q/A
Hit Ctrl+Enter in a BMP diff inline editor to save a comment.
-- 
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2014-11-24 21:01:04 +0000
+++ lib/canonical/launchpad/icing/style.css	2014-11-27 21:59:04 +0000
@@ -237,6 +237,11 @@
     width:2%;
     border-left: 1px solid #ddd;
 }
+.boardCommentDetails .editorShortcutTip {
+    float: right;
+    font-size: 0.9em;
+    color: gray;
+}
 .boardCommentBody {padding: 0.5em 12px 0;}
 .boardCommentActivity {
   padding: 0.5em 12px;

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-08-19 04:55:24 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-11-27 21:59:04 +0000
@@ -85,10 +85,19 @@
             namespace.flush_drafts_to_server();
             handling_request = false;
             draft_div.one('.boardCommentFooter').show();
+            draft_div.one('.editorShortcutTip').remove();
+        });
+        widget.editor.on('keydown', function(e) {
+           var enterKeyCode = 13;
+           if (e.domEvent.ctrlKey === true && 
+               e.domEvent.button === enterKeyCode) {
+               this.save();
+           } 
         });
         widget.editor.on('cancel', function(e) {
             handling_request = false;
             draft_div.one('.boardCommentFooter').show();
+            draft_div.one('.editorShortcutTip').remove();
             // If there's no saved comment to return to, just remove the
             // draft UI.
             if (namespace.inlinecomments[line_number] === undefined) {
@@ -98,6 +107,11 @@
         });
         widget._triggerEdit(e);
         draft_div.one('.boardCommentFooter').hide();
+        draft_div.one('.boardCommentDetails')
+          .append('<div class="editorShortcutTip">' +
+                  '[ESC] Cancel, [Ctrl + Enter] Save' +
+                  '</div>');
+
     };
     var delete_handler = function(e) {
         var line_number = namespace.find_line_number(e.currentTarget);

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-08-19 04:55:24 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-11-27 21:59:04 +0000
@@ -181,6 +181,28 @@
             Y.Assert.areEqual('diff-line-2', line.next().get('id'));
         },
 
+        test_edit_saving_via_shortcut: function() {
+            // Test that inline comments can be saved using the 
+            // ctrl + enter keyboard shortcut.
+            module.add_doubleclick_handler();
+            var line = Y.one('#diff-line-1'),
+                enterKeyCode = 13;
+            line.simulate('dblclick');
+            ic_area = line.next();
+            ic_area.one('.yui3-ieditor-input>textarea')
+              .set('value', 'test comment');
+            ic_area.one('.lazr-pos').simulate('keypress',
+                                              { charCode: enterKeyCode,
+                                                ctrlKey: true });
+            this.wait(function() {}, 1);
+            Y.Assert.areEqual(
+                'Unsaved comment',
+                ic_area.one('.boardCommentDetails').get('text'));
+            Y.Assert.areEqual(
+                'test comment',
+                ic_area.one('.yui3-editable_text-text').get('text'));
+        },
+
         test_logged_in: function() {
             var called = false;
             add_doubleclick_handler = function() {