launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/publish-ic-link into lp:launchpad.

Commit message:
Creating a link_scroller to take users from the end of the diff up to the review comment form with proper focus.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cprov/launchpad/publish-ic-link/+merge/220149

Creating a link_scroller at the bottom MP pages to take users to the review comment form.
It will automatically focus the comment textarea if there is no draft comments or the submit button if there are drafts.
 
-- 
https://code.launchpad.net/~cprov/launchpad/publish-ic-link/+merge/220149
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/publish-ic-link into lp:launchpad.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-15 04:18:54 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-20 02:56:57 +0000
@@ -499,6 +499,22 @@
                 self._showPreviewDiff(previewdiff_id);
             });
         });
+
+        var rc_scroller = Y.one('.review-comment-scroller');
+        if (rc_scroller !== null) {
+            return;
+        }
+        rc_scroller = Y.Node.create('<a href="" />')
+            .addClass('review-comment-scroller')
+            .set('text', 'Back to top of diff');
+        this.get('srcNode').append(rc_scroller);
+        rc.link_scroller(rc_scroller, '#add-comment-form', function() {
+            if (Y.all('.draft').size() > 0) {
+                Y.one('#add-comment-form input[type=submit]').focus();
+            } else {
+                Y.one('#add-comment-form textarea').focus();
+            }
+        });
     },
 
     /**
@@ -574,10 +590,13 @@
             return;
         }
 
-        var pub_drafts_container = Y.Node.create(
-            '<div class="publish_drafts_container">');
-        Y.one('[id="field.review_type"]').insert(
-             pub_drafts_container, 'after');
+	var pub_drafts_container = Y.one('.publish_drafts_container')
+        if (pub_drafts_container === null) {
+            pub_drafts_container = Y.Node.create(
+                '<div class="publish_drafts_container">');
+            Y.one('[id="field.review_type"]').insert(
+                pub_drafts_container, 'after');
+        }
         (new namespace.PublishDrafts(
             {'contentBox': pub_drafts_container})).render();
 

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html	2014-05-14 20:03:00 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html	2014-05-20 02:56:57 +0000
@@ -72,7 +72,8 @@
             <li>lp.code.branchmergeproposal.inlinecomments.test</li>
         </ul>
 
-      <div>
+      <div id="add-comment-form">
+        <textarea id="field.comment"></textarea>
         <input id="field.review_type" type="text"></input>
         <input id="field.actions.add" type="submit" value="Save"></input>
       </div>

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-15 03:47:40 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-20 02:56:57 +0000
@@ -524,6 +524,46 @@
             Y.Assert.areEqual(202, called_pd_id);
         },
 
+        test_diff_nav_publish_scroller: function() {
+            // The Diff Navigator publish comment *scroller* allows
+            // user to got from the bottom of the diff to the review
+            // comment form and is updated according to the existence
+            // of draft inline comments.
+            this.diffnav.render();
+            this.reply_previewdiffs();
+            this.reply_diffcontent();
+            var mockio = this.diffnav.get('lp_client').io_provider;
+            Y.Assert.areEqual(2, mockio.requests.length);
+            // We need to re-instrument the scroller in other to
+            // instantly fire the 'end' event (which runs the code
+            // that matters to us).
+            scroller = Y.one('.review-comment-scroller');
+            scroller.on('click', function() {
+                var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+                rc.window_scroll_anim.fire('end');
+            });
+            scroller.simulate('click');
+            // When there are no drafts, it scrolls to the form and focus
+            // on the comment textarea.
+            Y.Assert.areEqual(
+                'Back to top of diff', scroller.get('text'));
+            Y.Assert.areEqual('field.comment', document.activeElement.id);
+            // Create a draft inline comment and trigger an UI update.
+            module.create_row({'line_number': '2', 'text': 'another'});
+            Y.fire('inlinecomment.UPDATED');
+            // See above ...
+            scroller = Y.one('.review-comment-scroller');
+            scroller.on('click', function() {
+                var rc = Y.lp.code.branchmergeproposal.reviewcomment;
+                rc.window_scroll_anim.fire('end');
+            });
+            scroller.simulate('click');
+            // When there are draft, it scrolls to form and focus on the
+            // submit button.
+            Y.Assert.areEqual(
+               'field.actions.add', document.activeElement.id);
+        },
+
         test_diff_nav_failed_diff_content: function() {
             // An error message is presented when the Diff Navigator
             // fails to fetch the selected diff contents.