launchpad-reviewers team mailing list archive

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

replied to inline comments

Diff comments:

> === 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-06-01 00:26:09 +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
>      };
> @@ -96,6 +98,7 @@
>              handling_request = false;
>              draft_div.one('.boardCommentFooter').show();
>              draft_div.one('.editorShortcutTip').remove();
> +            namespace.init_keynav();

Yes, possible the input retains focus somehow - I'll see if I can blur() the commentbox on save.

>          });
>          widget.editor.on('keydown', function(e) {
>             if (e.domEvent.ctrlKey === true &&
> @@ -157,7 +160,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,12 +720,15 @@
>  });
>  
>  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];
>  
>      namespace.headers = namespace.get_headers();
> +    namespace.set_comments(namespace.headers);
>      namespace.scrollPos = -1;
>  
>      if (!namespace.handle_nav_keys_attached) {
> @@ -734,7 +740,22 @@
>  
>  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.set_comments = function(headers) {
> +    // Set first and last comment indicies, allowing for nav fallthough.
> +    for (var i = 0; i < headers.length; i++) {
> +        if (!namespace.comment_idx
> +            && namespace.header_has_type(headers[i], 'comment')) {
> +                namespace.comment_idx = {'first':  i};
> +            }
> +        if (namespace.comment_idx
> +            && namespace.header_has_type(headers[i], 'comment')) {
> +            namespace.comment_idx['last'] = i;
> +        }
> +    }

The first and last elements aren't necessarily comments (the headers array contains file, chunk and comment headers). This is a little inefficient and it might be better to create a map of headers to type on init, but that would require refactoring the logic elsewhere. For such a small collection, the user certainly isn't going to notice any ui blocking.

>  };
>  
>  namespace.add_nav_cursor = function(headers, row) {
> @@ -765,6 +786,10 @@
>      if (direction === 'forwards') {
>          for (i=cursor + 1; i < headers.length; i++) {
>              if (namespace.header_has_type(headers[i], type)) {
> +                if (type === 'comment' &&
> +                    typeof namespace.comment_idx === 'undefined') {
> +                    namespace.comment_idx = i;

err quite, that was from an earlier revision and should be gone!

> +                }
>                  return namespace.get_header_row(headers, i);
>              }
>          }
> @@ -776,11 +801,22 @@
>              }
>          }
>      }
> -    // fallthrough case sets state to first header or chunk.
> -    if (type === 'file') {
> -        return namespace.get_header_row(headers, 0);
> -    } else {
> +    // fallthrough case sets state to first header, chunk or comment.
> +    switch (type) {
> +    case 'comment':
> +        if (namespace.comment_idx) {
> +            if (direction === 'forwards') {
> +                return namespace.get_header_row(headers,
> +                                                namespace.comment_idx['first']);
> +            } else {
> +                return namespace.get_header_row(headers,
> +                                                namespace.comment_idx['last']);
> +            }
> +        }
> +    case 'chunk':
>          return namespace.get_header_row(headers, 1);
> +    default:
> +        return namespace.get_header_row(headers, 0);
>      }
>  };
>  
> @@ -788,7 +824,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 +842,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-06-01 00:26:09 +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-06-01 00:26:09 +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">
> 


-- 
https://code.launchpad.net/~blr/launchpad/inline-comment-nav/+merge/260427
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.