launchpad-reviewers team mailing list archive

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

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

Commit message:
Add inline comment navigation to branch merge proposal preview diffs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~blr/launchpad/inline-comment-nav/+merge/260427
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~blr/launchpad/inline-comment-nav into lp:launchpad.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2015-04-09 05:17:04 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2015-05-29 00:05:41 +0000
@@ -12,8 +12,10 @@
 if (typeof window.KeyEvent === "undefined") {
     window.KeyEvent = {
         DOM_VK_RETURN: 13,
+        DOM_VK_H: 72,
         DOM_VK_J: 74,
         DOM_VK_K: 75,
+        DOM_VK_L: 76,
         DOM_VK_N: 78,
         DOM_VK_P: 80
     };
@@ -157,7 +159,7 @@
     if (comments_tr === null) {
         comments_tr = Y.Node.create(
             '<tr class="inline-comments">' +
-            '<td colspan="2"><div></div></td></tr>')
+            '<td class="inline-comment" colspan="2"><div></div></td></tr>')
             .set('id', 'comments-diff-line-' + comment.line_number);
         Y.one('#diff-line-' + comment.line_number)
             .insert(comments_tr, 'after');
@@ -717,8 +719,10 @@
 });
 
 namespace.init_keynav = function() {
-    var diff_nav_keys = [window.KeyEvent.DOM_VK_J,
+    var diff_nav_keys = [window.KeyEvent.DOM_VK_H,
+                         window.KeyEvent.DOM_VK_J,
                          window.KeyEvent.DOM_VK_K,
+                         window.KeyEvent.DOM_VK_L,
                          window.KeyEvent.DOM_VK_N,
                          window.KeyEvent.DOM_VK_P];
 
@@ -734,7 +738,8 @@
 
 namespace.get_headers = function() {
     return document.querySelectorAll(
-        'table.diff td.diff-file, table.diff td.diff-chunk');
+        'table.diff td.diff-file, table.diff td.diff-chunk, ' +
+        'table.diff td.inline-comment');
 };
 
 namespace.add_nav_cursor = function(headers, row) {
@@ -788,7 +793,9 @@
     if (namespace.headers.length > 0) {
         var header = namespace.get_next_header(
             namespace.headers, type, direction);
-        namespace.add_nav_cursor(namespace.headers, header.row);
+        if (type !== 'comment') {
+            namespace.add_nav_cursor(namespace.headers, header.row);
+        }
         namespace.scrollPos = header.pos;
         header.row.scrollIntoView({block: 'start', behavior: 'smooth'});
     }
@@ -804,6 +811,12 @@
         document.querySelector(":focus")) {
         return;
     }
+    if (e.button === window.KeyEvent.DOM_VK_H) {
+        namespace.scroll_to_header('comment', 'forwards');
+    }
+    if (e.button === window.KeyEvent.DOM_VK_L) {
+        namespace.scroll_to_header('comment', 'backwards');
+    }
     if (e.button === window.KeyEvent.DOM_VK_J) {
         namespace.scroll_to_header('file', 'forwards');
     }

=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-03-27 00:26:02 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-05-29 00:05:41 +0000
@@ -622,7 +622,7 @@
 The text of the review diff is in the page.
 
     >>> print repr(extract_text(get_review_diff()))
-    u'Preview Diff\n[J/K] Next/Prev File, [N/P] Next/Prev Hunk\nDownload diff\n1\n---\n2\n+++\n3\n@@ -0,0 +1 @@\n4\n+Fake Diff\u1010'
+    u'Preview Diff\n[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk\nDownload diff\n1\n---\n2\n+++\n3\n@@ -0,0 +1 @@\n4\n+Fake Diff\u1010'
 
 There is also a link to the diff URL, which is the preview diff URL plus
 "+files/preview.diff". It redirects logged in users to the file in the

=== modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
--- lib/lp/code/templates/branchmergeproposal-diff.pt	2015-03-26 21:18:17 +0000
+++ lib/lp/code/templates/branchmergeproposal-diff.pt	2015-05-29 00:05:41 +0000
@@ -6,7 +6,7 @@
        tal:define="attachment view/preview_diff/diff_text">
     <tal:real-diff condition="attachment">
       <div class="boardCommentDetails filename">
-        <div class="editorShortcutTip">[J/K] Next/Prev File, [N/P] Next/Prev Hunk</div>
+        <div class="editorShortcutTip">[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk</div>
         <ul class="horizontal">
           <li>
             <a tal:attributes="href view/preview_diff/fmt:url">