Launchpad itself

Merge lp:~cjohnston/launchpad/diff-return-text into lp:launchpad

  1. diff-return-text
  2. Merge into devel
Proposed by Chris Johnston
Status: Merged
Merged at revision: 17019
Proposed branch: lp:~cjohnston/launchpad/diff-return-text
Merge into: lp:launchpad
Diff against target: 57 lines (+19/-3)
2 files modified
lib/lp/code/javascript/branchmergeproposal.inlinecomments.js (+11/-2)
lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js (+8/-1)
To merge this branch: bzr merge lp:~cjohnston/launchpad/diff-return-text
Reviewer Review Type Date Requested Status
Celso Providelo (community) Approve
Review via email: mp+220954@code.launchpad.net

Commit message

Update text below diff to push a user to return and save their ICs.

Description of the change

Some users were finding that it wasn't obvious that you had to go back to save your comments. This will update the text below the diff to help prompt a user to return and save their comments.

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

Chris, the change looks good and we can land it now and benefit from the wider audience feedback.

Later on, when re-structuring the whole JS code, it looks like we should move rc_scroller creation from DiffNav() to the PublishDraft() domain, where it is updated.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
2--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-05-21 09:09:28 +0000
3+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-05-26 13:09:20 +0000
4@@ -268,6 +268,16 @@
5 syncUI: function() {
6 this.get('contentBox').empty();
7 var n_drafts = Y.all('.draft').size();
8+ var rc_scroller = Y.one('.review-comment-scroller');
9+ if (rc_scroller !== null) {
10+ if (n_drafts > 0) {
11+ var scroller_text = 'Return to save comment'
12+ if (n_drafts > 1) scroller_text += 's';
13+ rc_scroller.set('text', scroller_text);
14+ } else {
15+ rc_scroller.set('text', 'Return to add comment');
16+ }
17+ }
18 if (n_drafts == 0) {
19 Y.fire('CodeReviewComment.SET_DISABLED', true);
20 return;
21@@ -502,9 +512,8 @@
22 if (rc_scroller !== null) {
23 return;
24 }
25- rc_scroller = Y.Node.create('<a href="" />')
26+ rc_scroller = Y.Node.create('<a href="">Return to add comment</a>')
27 .addClass('review-comment-scroller')
28- .set('text', 'Back to top of diff');
29 this.get('srcNode').append(rc_scroller);
30 rc.link_scroller(rc_scroller, '#add-comment-form', function() {
31 if (Y.all('.draft').size() > 0) {
32
33=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
34--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-05-21 09:33:04 +0000
35+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-05-26 13:09:20 +0000
36@@ -552,13 +552,20 @@
37 scroller.simulate('click');
38 // When there are no drafts, it scrolls to the form and focus
39 // on the comment textarea.
40+ Y.fire('inlinecomment.UPDATED');
41 Y.Assert.areEqual(
42- 'Back to top of diff', scroller.get('text'));
43+ 'Return to add comment', scroller.get('text'));
44 Y.Assert.areEqual('field.comment', document.activeElement.id);
45 // Create a draft inline comment and trigger an UI update.
46 module.create_row({'line_number': '2', 'text': 'another'});
47 Y.fire('inlinecomment.UPDATED');
48 // See above ...
49+ Y.Assert.areEqual(
50+ 'Return to save comment', scroller.get('text'));
51+ module.create_row({'line_number': '3', 'text': 'and again'});
52+ Y.fire('inlinecomment.UPDATED');
53+ Y.Assert.areEqual(
54+ 'Return to save comments', scroller.get('text'));
55 scroller = Y.one('.review-comment-scroller');
56 scroller.on('click', function() {
57 var rc = Y.lp.code.branchmergeproposal.reviewcomment;