launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Review: Needs Fixing

Looks good, thanks Chris.

I just think we should verify that tests (and feature) work without recreating the scroller on updates. There is no reason why it shouldn't since the 'end'ing code checks for draft(s) in runtime.


Inline comments:

> --- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-15 04:18:54 +0000
> +++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-19 22:41:25 +0000
> @@ -499,6 +499,22 @@
>                  self._showPreviewDiff(previewdiff_id);
>              });
>          });
> +
> +        var rc_scroller = Y.one('.review-comment-scroller');
> +        if (rc_scroller !== null) {
> +            rc_scroller.remove(true);
> +        }

I don't think we need to re-create the scroller every time. The original implementation if (scroller !== null) return; should work fine here.

> +        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();
> +            }
> +        });
>      },
>  
>      /**
> 
> === 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-19 22:41:25 +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-19 22:41:25 +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.
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/publish-ic-link/+merge/220103
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.