Merge lp:~cjohnston/launchpad/publish-ic-link into lp:launchpad

Proposed by Chris Johnston
Status: Merged
Merged at revision: 17011
Proposed branch: lp:~cjohnston/launchpad/publish-ic-link
Merge into: lp:launchpad
Diff against target: 91 lines (+58/-1)
3 files modified
lib/lp/code/javascript/branchmergeproposal.inlinecomments.js (+16/-0)
lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html (+2/-1)
lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js (+40/-0)
To merge this branch: bzr merge lp:~cjohnston/launchpad/publish-ic-link
Reviewer Review Type Date Requested Status
Celso Providelo (community) Needs Fixing
Review via email: mp+220103@code.launchpad.net

Commit message

Add link below diff to return back to the top of diff (Add comment section) via scrollers.

To post a comment you must log in.
Revision history for this message
Celso Providelo (cprov) wrote :

Please address the issues pointed as inline comments and we can have another review-round.

Revision history for this message
Celso Providelo (cprov) :
review: Needs Fixing
Revision history for this message
Celso Providelo (cprov) wrote :

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.

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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-19 22:41:25 +0000
@@ -499,6 +499,22 @@
499 self._showPreviewDiff(previewdiff_id);499 self._showPreviewDiff(previewdiff_id);
500 });500 });
501 });501 });
502
503 var rc_scroller = Y.one('.review-comment-scroller');
504 if (rc_scroller !== null) {
505 rc_scroller.remove(true);
506 }
507 rc_scroller = Y.Node.create('<a href="" />')
508 .addClass('review-comment-scroller')
509 .set('text', 'Back to top of diff');
510 this.get('srcNode').append(rc_scroller);
511 rc.link_scroller(rc_scroller, '#add-comment-form', function() {
512 if (Y.all('.draft').size() > 0) {
513 Y.one('#add-comment-form input[type=submit]').focus();
514 } else {
515 Y.one('#add-comment-form textarea').focus();
516 }
517 });
502 },518 },
503519
504 /**520 /**
505521
=== 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 @@
72 <li>lp.code.branchmergeproposal.inlinecomments.test</li>72 <li>lp.code.branchmergeproposal.inlinecomments.test</li>
73 </ul>73 </ul>
7474
75 <div>75 <div id="add-comment-form">
76 <textarea id="field.comment"></textarea>
76 <input id="field.review_type" type="text"></input>77 <input id="field.review_type" type="text"></input>
77 <input id="field.actions.add" type="submit" value="Save"></input>78 <input id="field.actions.add" type="submit" value="Save"></input>
78 </div>79 </div>
7980
=== 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 @@
524 Y.Assert.areEqual(202, called_pd_id);524 Y.Assert.areEqual(202, called_pd_id);
525 },525 },
526526
527 test_diff_nav_publish_scroller: function() {
528 // The Diff Navigator publish comment *scroller* allows
529 // user to got from the bottom of the diff to the review
530 // comment form and is updated according to the existence
531 // of draft inline comments.
532 this.diffnav.render();
533 this.reply_previewdiffs();
534 this.reply_diffcontent();
535 var mockio = this.diffnav.get('lp_client').io_provider;
536 Y.Assert.areEqual(2, mockio.requests.length);
537 // We need to re-instrument the scroller in other to
538 // instantly fire the 'end' event (which runs the code
539 // that matters to us).
540 scroller = Y.one('.review-comment-scroller');
541 scroller.on('click', function() {
542 var rc = Y.lp.code.branchmergeproposal.reviewcomment;
543 rc.window_scroll_anim.fire('end');
544 });
545 scroller.simulate('click');
546 // When there are no drafts, it scrolls to the form and focus
547 // on the comment textarea.
548 Y.Assert.areEqual(
549 'Back to top of diff', scroller.get('text'));
550 Y.Assert.areEqual('field.comment', document.activeElement.id);
551 // Create a draft inline comment and trigger an UI update.
552 module.create_row({'line_number': '2', 'text': 'another'});
553 Y.fire('inlinecomment.UPDATED');
554 // See above ...
555 scroller = Y.one('.review-comment-scroller');
556 scroller.on('click', function() {
557 var rc = Y.lp.code.branchmergeproposal.reviewcomment;
558 rc.window_scroll_anim.fire('end');
559 });
560 scroller.simulate('click');
561 // When there are draft, it scrolls to form and focus on the
562 // submit button.
563 Y.Assert.areEqual(
564 'field.actions.add', document.activeElement.id);
565 },
566
527 test_diff_nav_failed_diff_content: function() {567 test_diff_nav_failed_diff_content: function() {
528 // An error message is presented when the Diff Navigator568 // An error message is presented when the Diff Navigator
529 // fails to fetch the selected diff contents.569 // fails to fetch the selected diff contents.