Merge lp:~blr/launchpad/inline-comment-delint into lp:launchpad
- inline-comment-delint
- Merge into devel
Proposed by
Kit Randel
Status: | Merged |
---|---|
Approved by: | Kit Randel |
Approved revision: | no longer in the source branch. |
Merged at revision: | 17311 |
Proposed branch: | lp:~blr/launchpad/inline-comment-delint |
Merge into: | lp:launchpad |
Diff against target: |
233 lines (+38/-34) 1 file modified
lib/lp/code/javascript/branchmergeproposal.inlinecomments.js (+38/-34) |
To merge this branch: | bzr merge lp:~blr/launchpad/inline-comment-delint |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Review via email: mp+243322@code.launchpad.net |
Commit message
Description of the change
A delinting pass over the inline comment code I've been looking at recently.
To post a comment you must log in.
Revision history for this message
William Grant (wgrant) : | # |
review:
Approve
(code)
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-11-27 22:27:28 +0000 | |||
3 | +++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-12-01 19:25:34 +0000 | |||
4 | @@ -43,7 +43,7 @@ | |||
5 | 43 | namespace.flush_drafts_to_server = function() { | 43 | namespace.flush_drafts_to_server = function() { |
6 | 44 | var config = { | 44 | var config = { |
7 | 45 | on: { | 45 | on: { |
9 | 46 | success: function () { | 46 | success: function() { |
10 | 47 | Y.fire('inlinecomment.UPDATED'); | 47 | Y.fire('inlinecomment.UPDATED'); |
11 | 48 | } | 48 | } |
12 | 49 | }, | 49 | }, |
13 | @@ -89,10 +89,10 @@ | |||
14 | 89 | }); | 89 | }); |
15 | 90 | widget.editor.on('keydown', function(e) { | 90 | widget.editor.on('keydown', function(e) { |
16 | 91 | var enterKeyCode = 13; | 91 | var enterKeyCode = 13; |
18 | 92 | if (e.domEvent.ctrlKey === true && | 92 | if (e.domEvent.ctrlKey === true && |
19 | 93 | e.domEvent.button === enterKeyCode) { | 93 | e.domEvent.button === enterKeyCode) { |
20 | 94 | this.save(); | 94 | this.save(); |
22 | 95 | } | 95 | } |
23 | 96 | }); | 96 | }); |
24 | 97 | widget.editor.on('cancel', function(e) { | 97 | widget.editor.on('cancel', function(e) { |
25 | 98 | handling_request = false; | 98 | handling_request = false; |
26 | @@ -111,7 +111,6 @@ | |||
27 | 111 | .append('<div class="editorShortcutTip">' + | 111 | .append('<div class="editorShortcutTip">' + |
28 | 112 | '[Esc] Cancel, [Ctrl+Enter] Save' + | 112 | '[Esc] Cancel, [Ctrl+Enter] Save' + |
29 | 113 | '</div>'); | 113 | '</div>'); |
30 | 114 | |||
31 | 115 | }; | 114 | }; |
32 | 116 | var delete_handler = function(e) { | 115 | var delete_handler = function(e) { |
33 | 117 | var line_number = namespace.find_line_number(e.currentTarget); | 116 | var line_number = namespace.find_line_number(e.currentTarget); |
34 | @@ -143,11 +142,14 @@ | |||
35 | 143 | 142 | ||
36 | 144 | namespace.create_row = function(comment) { | 143 | namespace.create_row = function(comment) { |
37 | 145 | // Find or create the container for this line's comments. | 144 | // Find or create the container for this line's comments. |
39 | 146 | var comments_tr = Y.one('#comments-diff-line-' + comment.line_number); | 145 | var comments_tr = Y.one('#comments-diff-line-' + |
40 | 146 | comment.line_number), | ||
41 | 147 | comment_date; | ||
42 | 148 | |||
43 | 147 | if (comments_tr === null) { | 149 | if (comments_tr === null) { |
47 | 148 | var comments_tr = Y.Node.create( | 150 | comments_tr = Y.Node.create( |
48 | 149 | '<tr class="inline-comments">' | 151 | '<tr class="inline-comments">' + |
49 | 150 | + '<td colspan="2"><div></div></td></tr>') | 152 | '<td colspan="2"><div></div></td></tr>') |
50 | 151 | .set('id', 'comments-diff-line-' + comment.line_number); | 153 | .set('id', 'comments-diff-line-' + comment.line_number); |
51 | 152 | Y.one('#diff-line-' + comment.line_number) | 154 | Y.one('#diff-line-' + comment.line_number) |
52 | 153 | .insert(comments_tr, 'after'); | 155 | .insert(comments_tr, 'after'); |
53 | @@ -181,7 +183,7 @@ | |||
54 | 181 | // Wrap resources coming from LP.cache, as they come from | 183 | // Wrap resources coming from LP.cache, as they come from |
55 | 182 | // the LP API (updates during the page life-span). This way | 184 | // the LP API (updates during the page life-span). This way |
56 | 183 | // the handling code can be unified. | 185 | // the handling code can be unified. |
58 | 184 | if (LP.links['me'] !== undefined) { | 186 | if (LP.links.me !== undefined) { |
59 | 185 | newrow.append('<div class="boardCommentFooter"></div>'); | 187 | newrow.append('<div class="boardCommentFooter"></div>'); |
60 | 186 | newrow.one('.boardCommentFooter') | 188 | newrow.one('.boardCommentFooter') |
61 | 187 | .append('<a class="js-action draft-edit">Reply</a>'); | 189 | .append('<a class="js-action draft-edit">Reply</a>'); |
62 | @@ -198,10 +200,10 @@ | |||
63 | 198 | // XXX cprov 20140226: the date should be formatted according to | 200 | // XXX cprov 20140226: the date should be formatted according to |
64 | 199 | // the user locale/timezone similar to fmt:displaydate. | 201 | // the user locale/timezone similar to fmt:displaydate. |
65 | 200 | // We should leverage Y.Intl features. | 202 | // We should leverage Y.Intl features. |
68 | 201 | if (typeof comment.date === "string") { | 203 | if (typeof comment.date === 'string') { |
69 | 202 | var comment_date = Y.lp.app.date.parse_date(comment.date); | 204 | comment_date = Y.lp.app.date.parse_date(comment.date); |
70 | 203 | } else { | 205 | } else { |
72 | 204 | var comment_date = comment.date; | 206 | comment_date = comment.date; |
73 | 205 | } | 207 | } |
74 | 206 | var date = Y.lp.app.date.approximatedate(comment_date); | 208 | var date = Y.lp.app.date.approximatedate(comment_date); |
75 | 207 | headerspan.one('span').set('text', date); | 209 | headerspan.one('span').set('text', date); |
76 | @@ -228,11 +230,11 @@ | |||
77 | 228 | namespace.populate_comments = function() { | 230 | namespace.populate_comments = function() { |
78 | 229 | var config_published = { | 231 | var config_published = { |
79 | 230 | on: { | 232 | on: { |
81 | 231 | success: function (comments) { | 233 | success: function(comments) { |
82 | 232 | // Display published inline comments. | 234 | // Display published inline comments. |
83 | 233 | // [{'line_number': <lineno>, 'person': <IPerson>, | 235 | // [{'line_number': <lineno>, 'person': <IPerson>, |
84 | 234 | // 'text': <comment>, 'date': <timestamp>}, ...] | 236 | // 'text': <comment>, 'date': <timestamp>}, ...] |
86 | 235 | comments.forEach( function (comment) { | 237 | comments.forEach(function(comment) { |
87 | 236 | namespace.create_row(comment); | 238 | namespace.create_row(comment); |
88 | 237 | }); | 239 | }); |
89 | 238 | Y.fire('inlinecomment.UPDATED'); | 240 | Y.fire('inlinecomment.UPDATED'); |
90 | @@ -249,14 +251,14 @@ | |||
91 | 249 | namespace.populate_drafts = function() { | 251 | namespace.populate_drafts = function() { |
92 | 250 | var config_draft = { | 252 | var config_draft = { |
93 | 251 | on: { | 253 | on: { |
95 | 252 | success: function (drafts) { | 254 | success: function(drafts) { |
96 | 253 | namespace.inlinecomments = {}; | 255 | namespace.inlinecomments = {}; |
97 | 254 | if (drafts === null) { | 256 | if (drafts === null) { |
98 | 255 | return; | 257 | return; |
99 | 256 | } | 258 | } |
100 | 257 | // Display draft inline comments: | 259 | // Display draft inline comments: |
101 | 258 | // {'<line_number>':''<comment>', ...}) | 260 | // {'<line_number>':''<comment>', ...}) |
103 | 259 | Object.keys(drafts).forEach( function (line_number) { | 261 | Object.keys(drafts).forEach(function(line_number) { |
104 | 260 | var comment = { | 262 | var comment = { |
105 | 261 | 'line_number': line_number, | 263 | 'line_number': line_number, |
106 | 262 | 'text': drafts[line_number] | 264 | 'text': drafts[line_number] |
107 | @@ -284,7 +286,7 @@ | |||
108 | 284 | // loaded for anonymous requests. | 286 | // loaded for anonymous requests. |
109 | 285 | // In fact, if loading draft is attempted is will fail due to | 287 | // In fact, if loading draft is attempted is will fail due to |
110 | 286 | // the LP permission checks. | 288 | // the LP permission checks. |
112 | 287 | if (LP.links['me'] !== undefined) { | 289 | if (LP.links.me !== undefined) { |
113 | 288 | // Add the double-click handler for each row in the diff. This needs | 290 | // Add the double-click handler for each row in the diff. This needs |
114 | 289 | // to be done first since populating existing published and draft | 291 | // to be done first since populating existing published and draft |
115 | 290 | // comments may add more rows. | 292 | // comments may add more rows. |
116 | @@ -295,7 +297,7 @@ | |||
117 | 295 | }; | 297 | }; |
118 | 296 | 298 | ||
119 | 297 | 299 | ||
121 | 298 | var PublishDrafts = function () { | 300 | var PublishDrafts = function() { |
122 | 299 | PublishDrafts.superclass.constructor.apply(this, arguments); | 301 | PublishDrafts.superclass.constructor.apply(this, arguments); |
123 | 300 | }; | 302 | }; |
124 | 301 | 303 | ||
125 | @@ -320,29 +322,31 @@ | |||
126 | 320 | * @method syncUI | 322 | * @method syncUI |
127 | 321 | */ | 323 | */ |
128 | 322 | syncUI: function() { | 324 | syncUI: function() { |
129 | 325 | var n_drafts = Y.all('.draft').size(), | ||
130 | 326 | rc_scroller = Y.one('.review-comment-scroller'), | ||
131 | 327 | scroller_text = ''; | ||
132 | 328 | |||
133 | 323 | this.get('contentBox').empty(); | 329 | this.get('contentBox').empty(); |
134 | 324 | var n_drafts = Y.all('.draft').size(); | ||
135 | 325 | var rc_scroller = Y.one('.review-comment-scroller'); | ||
136 | 326 | if (rc_scroller !== null) { | 330 | if (rc_scroller !== null) { |
137 | 327 | if (n_drafts > 0) { | 331 | if (n_drafts > 0) { |
140 | 328 | var scroller_text = 'Return to save comment' | 332 | scroller_text = 'Return to save comment'; |
141 | 329 | if (n_drafts > 1) scroller_text += 's'; | 333 | if (n_drafts > 1) { scroller_text += 's'; } |
142 | 330 | rc_scroller.set('text', scroller_text); | 334 | rc_scroller.set('text', scroller_text); |
143 | 331 | } else { | 335 | } else { |
144 | 332 | rc_scroller.set('text', 'Return to add comment'); | 336 | rc_scroller.set('text', 'Return to add comment'); |
145 | 333 | } | 337 | } |
146 | 334 | } | 338 | } |
148 | 335 | if (n_drafts == 0) { | 339 | if (n_drafts === 0) { |
149 | 336 | Y.fire('CodeReviewComment.SET_DISABLED', true); | 340 | Y.fire('CodeReviewComment.SET_DISABLED', true); |
150 | 337 | return; | 341 | return; |
151 | 338 | } | 342 | } |
152 | 339 | var text = ' Include ' + n_drafts + ' diff comment'; | 343 | var text = ' Include ' + n_drafts + ' diff comment'; |
154 | 340 | if (n_drafts > 1) text += 's'; | 344 | if (n_drafts > 1) { text += 's'; } |
155 | 341 | var checkbox = Y.Node.create( | 345 | var checkbox = Y.Node.create( |
156 | 342 | '<input id="field.publish_inline_comments"' + | 346 | '<input id="field.publish_inline_comments"' + |
157 | 343 | 'name="field.publish_inline_comments"' + | 347 | 'name="field.publish_inline_comments"' + |
158 | 344 | 'type="checkbox" class="checkboxType"' + | 348 | 'type="checkbox" class="checkboxType"' + |
160 | 345 | 'checked=""></input>') | 349 | 'checked=""></input>'); |
161 | 346 | var label = Y.Node.create( | 350 | var label = Y.Node.create( |
162 | 347 | '<label for="field.publish_inline_comments" />') | 351 | '<label for="field.publish_inline_comments" />') |
163 | 348 | .set('text', text); | 352 | .set('text', text); |
164 | @@ -367,7 +371,7 @@ | |||
165 | 367 | namespace.PublishDrafts = PublishDrafts; | 371 | namespace.PublishDrafts = PublishDrafts; |
166 | 368 | 372 | ||
167 | 369 | 373 | ||
169 | 370 | var InlineCommentToggle = function () { | 374 | var InlineCommentToggle = function() { |
170 | 371 | InlineCommentToggle.superclass.constructor.apply(this, arguments); | 375 | InlineCommentToggle.superclass.constructor.apply(this, arguments); |
171 | 372 | }; | 376 | }; |
172 | 373 | 377 | ||
173 | @@ -408,7 +412,7 @@ | |||
174 | 408 | if (display) { | 412 | if (display) { |
175 | 409 | css_display = 'table-row'; | 413 | css_display = 'table-row'; |
176 | 410 | } | 414 | } |
178 | 411 | Y.all('.inline-comments').each( function() { | 415 | Y.all('.inline-comments').each(function() { |
179 | 412 | this.setStyle('display', css_display); | 416 | this.setStyle('display', css_display); |
180 | 413 | this.next().setStyle('display', css_display); | 417 | this.next().setStyle('display', css_display); |
181 | 414 | }); | 418 | }); |
182 | @@ -469,12 +473,12 @@ | |||
183 | 469 | * @method initializer | 473 | * @method initializer |
184 | 470 | * @protected | 474 | * @protected |
185 | 471 | */ | 475 | */ |
187 | 472 | initializer: function(cfg){ | 476 | initializer: function(cfg) { |
188 | 473 | // If we have not been provided with a Launchpad Client, then | 477 | // If we have not been provided with a Launchpad Client, then |
189 | 474 | // create one now: | 478 | // create one now: |
191 | 475 | if (null === this.get("lp_client")){ | 479 | if (null === this.get('lp_client')) { |
192 | 476 | // Create our own instance of the LP client. | 480 | // Create our own instance of the LP client. |
194 | 477 | this.set("lp_client", new Y.lp.client.Launchpad()); | 481 | this.set('lp_client', new Y.lp.client.Launchpad()); |
195 | 478 | } | 482 | } |
196 | 479 | }, | 483 | }, |
197 | 480 | 484 | ||
198 | @@ -534,7 +538,7 @@ | |||
199 | 534 | _connectScrollers: function() { | 538 | _connectScrollers: function() { |
200 | 535 | var self = this; | 539 | var self = this; |
201 | 536 | var rc = Y.lp.code.branchmergeproposal.reviewcomment; | 540 | var rc = Y.lp.code.branchmergeproposal.reviewcomment; |
203 | 537 | Y.all('td[data-previewdiff-id]').each( function(td) { | 541 | Y.all('td[data-previewdiff-id]').each(function(td) { |
204 | 538 | // Comments from superseded MPs should be ignored. | 542 | // Comments from superseded MPs should be ignored. |
205 | 539 | if (td.getData('from-superseded') === 'True') { | 543 | if (td.getData('from-superseded') === 'True') { |
206 | 540 | return; | 544 | return; |
207 | @@ -561,7 +565,7 @@ | |||
208 | 561 | return; | 565 | return; |
209 | 562 | } | 566 | } |
210 | 563 | rc_scroller = Y.Node.create('<a href="">Return to add comment</a>') | 567 | rc_scroller = Y.Node.create('<a href="">Return to add comment</a>') |
212 | 564 | .addClass('review-comment-scroller') | 568 | .addClass('review-comment-scroller'); |
213 | 565 | this.get('srcNode').append(rc_scroller); | 569 | this.get('srcNode').append(rc_scroller); |
214 | 566 | rc.link_scroller(rc_scroller, '#add-comment-form', function() { | 570 | rc.link_scroller(rc_scroller, '#add-comment-form', function() { |
215 | 567 | if (Y.all('.draft').size() > 0) { | 571 | if (Y.all('.draft').size() > 0) { |
216 | @@ -583,7 +587,7 @@ | |||
217 | 583 | var container = this.get('srcNode').one('.diff-content'); | 587 | var container = this.get('srcNode').one('.diff-content'); |
218 | 584 | var config = { | 588 | var config = { |
219 | 585 | on: { | 589 | on: { |
221 | 586 | success: function (diff_html) { | 590 | success: function(diff_html) { |
222 | 587 | container.set('innerHTML', diff_html); | 591 | container.set('innerHTML', diff_html); |
223 | 588 | self.set_status_updated(); | 592 | self.set_status_updated(); |
224 | 589 | }, | 593 | }, |
225 | @@ -642,7 +646,7 @@ | |||
226 | 642 | 646 | ||
227 | 643 | var pub_drafts_anchor = Y.one('[id="field.review_type"]'); | 647 | var pub_drafts_anchor = Y.one('[id="field.review_type"]'); |
228 | 644 | if (pub_drafts_anchor !== null) { | 648 | if (pub_drafts_anchor !== null) { |
230 | 645 | var pub_drafts_container = Y.one('.publish_drafts_container') | 649 | var pub_drafts_container = Y.one('.publish_drafts_container'); |
231 | 646 | if (pub_drafts_container === null) { | 650 | if (pub_drafts_container === null) { |
232 | 647 | pub_drafts_container = Y.Node.create( | 651 | pub_drafts_container = Y.Node.create( |
233 | 648 | '<div class="publish_drafts_container">'); | 652 | '<div class="publish_drafts_container">'); |