Launchpad itself

Merge lp:~thumper/launchpad/hiding-review-fields into lp:launchpad

  1. hiding-review-fields
  2. Merge into devel
Proposed by Tim Penhey
Status: Rejected
Rejected by: Tim Penhey
Proposed branch: lp:~thumper/launchpad/hiding-review-fields
Merge into: lp:launchpad
Diff against target: 271 lines (+127/-39)
4 files modified
lib/canonical/launchpad/javascript/code/codereview.js (+35/-2)
lib/canonical/launchpad/javascript/lp/comment.js (+4/-5)
lib/lp/code/browser/codereviewcomment.py (+3/-2)
lib/lp/code/templates/branchmergeproposal-index.pt (+85/-30)
To merge this branch: bzr merge lp:~thumper/launchpad/hiding-review-fields
Reviewer Review Type Date Requested Status
Michael Nelson (community) ui Needs Information
Paul Hummer (community) ui Approve
Martin Albisetti ui Pending
Review via email: mp+15722@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

I tried a branch where the review fields were visible all the time, but with a different empty review type option, but it looked really icky.

I've moved the expander links to be below the title, and added collapse icons.

I've also changed the default to leave a review, as that is what we'd like people to do more often.

Revision history for this message
Paul Hummer (rockstar) :
review: Approve (ui)
Revision history for this message
Michael Nelson (michael.nelson) wrote :
Download full text (3.1 KiB)

Hi Tim,

Great to see this being worked on, I've often thought when just adding a comment that ignoring a 'Review' outcome field (with '-Select-' selected) was a bit strange when it's not indicated as optional etc.

I say the following with full knowledge that you know your audience here better than I do, but:

1) I'm not convinced the changing show/hide in '[Show|Hide] review fields' is necessary? Wouldn't a static 'Optional review fields' be more consistent. My impression is that people understand the toggle is to show/hide.

2) It seems kind-of weird to me that toggling a drop-down changes other parts of the UI (ie. it *doesn't* just drop down some options). Actually, I'm not sure what the benefit of hiding them is if they are shown by default. (ie. I decided to add a comment only, so I hide the extra fields rather than ignoring them, which causes the UI to update a bit to my specific situation).

3) Just a thought: I wonder whether all of the complexity here could be removed by simply hiding everything initially, except for two links: (1) Add comment (2) Add review . When the user clicks on Add comment, the text area appears with the Save comment button. If instead they clicked on the 'Add review' link, the text area appears with the additional fields and a 'Save review' button. If the user changes their mind half-way through, they can still click on the other link (Add comment, or Add review) which will update the UI (kindof like GMail's reply/reply all - you can click reply all after already writing your reply etc.) The downside to this is that, unless you were happy for comment to be the default (which from what you've said above isn't a good idea), then it wouldn't be possible to display the textarea initially without confusing the UI. Interested to hear what you think.

4) Personally, I'd prefer the review fields *below* the text area. I often find myself doing the following:
  i) Choose a review (Approve) based on my initial thought.
  ii) Start writing the comment, but half way through changing my mind because of something I've discovered, so
  iii) Going back up and changing the Review outcome.
If the fields were after the comment, it would suite (my) workflow, but that could just be a personal thing. Another benefit is that in encourages (but doesn't force) people to write a comment *before* selecting the review outcome (hint, hint, Paul ;) ). Again, I'm interested to hear your thoughts there.

So, a simple alternative that incorporates (4) and requires very little JS would be:

http://people.canonical.com/~michaeln/tmp/codereview.png

where the normal '-Select-' default review option is replaced by 'Comment only', and the only JS required is to disable the review type field unless something other than 'Comment only' is selected. You mentioned above that you'd tried a branch where the fields were visible all the time, but didn't like it? I'm not sure how different my mockup above is from what you tried.

If on the other hand, you think that the fields definitely need to be hidden when adding a comment only, have a think about (3) above. If you hate both those options, then maybe Martin has more thoughts.

I hope that's helpful f...

Read more...

review: Needs Information (ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/javascript/code/codereview.js'
--- lib/canonical/launchpad/javascript/code/codereview.js 2009-11-30 23:51:34 +0000
+++ lib/canonical/launchpad/javascript/code/codereview.js 2009-12-09 08:04:13 +0000
@@ -17,13 +17,46 @@
17/*17/*
18 * Connect all the links to their given actions.18 * Connect all the links to their given actions.
19 */19 */
20Y.codereview.connect_links = function() {20Y.codereview.connect_links = function(comment_widget) {
2121
22 var link = Y.one('#request-review');22 var link = Y.one('#request-review');
23 if (link !== null) {23 if (link !== null) {
24 link.addClass('js-action');24 link.addClass('js-action');
25 link.on('click', show_request_review_form);25 link.on('click', show_request_review_form);
26 }26 }
27 // Show the link.
28 Y.one('#add-review a').removeClass('unseen');
29 var review_fields = Y.one('#add-comment-review-fields');
30 Y.one('#add-comment a').on('click', function(e) {
31 e.preventDefault();
32 Y.one('#add-comment').addClass('unseen');
33 Y.one('#add-review').removeClass('unseen');
34 review_fields.removeClass('unseen');
35 if (review_fields.anim !== undefined)
36 review_fields.anim.stop();
37 review_fields.anim = Y.lazr.effects.slide_out(review_fields);
38 review_fields.anim.run();
39 comment_widget.syncUI();
40 });
41 Y.one('#add-review a').on('click', function(e) {
42 e.preventDefault();
43 Y.one('#add-review').addClass('unseen');
44 Y.one('#add-comment').removeClass('unseen');
45
46 if (review_fields.anim !== undefined)
47 review_fields.anim.stop();
48 review_fields.anim = Y.lazr.effects.slide_in(review_fields);
49 review_fields.anim.on('end', function() {
50 review_fields.addClass('unseen'); });
51 review_fields.anim.run();
52
53 comment_widget.syncUI();
54 });
55 // Remove the empty option.
56 var review_votes = Y.one('#add-comment-review-fields select');
57 var empty_option = review_votes.query('option[value=""]');
58 review_votes.removeChild(empty_option);
59 // Hook up the commit message.
27 link = Y.one('.menu-link-set_commit_message');60 link = Y.one('.menu-link-set_commit_message');
28 if (link !== null) {61 if (link !== null) {
29 link.addClass('js-action');62 link.addClass('js-action');
@@ -221,4 +254,4 @@
221254
222Y.codereview.NumberToggle = NumberToggle;255Y.codereview.NumberToggle = NumberToggle;
223256
224}, "0.1", {"requires": ["base", "widget", "lazr.anim", "lazr.formoverlay", "lp.picker"]});257 }, "0.1", {"requires": ["base", "widget", "lazr.anim", "lazr.effects", "lazr.formoverlay", "lp.picker"]});
225258
=== modified file 'lib/canonical/launchpad/javascript/lp/comment.js'
--- lib/canonical/launchpad/javascript/lp/comment.js 2009-11-26 19:54:52 +0000
+++ lib/canonical/launchpad/javascript/lp/comment.js 2009-12-09 08:04:13 +0000
@@ -211,6 +211,7 @@
211 initializer: function() {211 initializer: function() {
212 this.vote_input = Y.one('[id="field.vote"]');212 this.vote_input = Y.one('[id="field.vote"]');
213 this.review_type = Y.one('[id="field.review_type"]');213 this.review_type = Y.one('[id="field.review_type"]');
214 this.review_fields = Y.one('#add-comment-review-fields');
214 this.in_reply_to = null;215 this.in_reply_to = null;
215 },216 },
216 /**217 /**
@@ -227,6 +228,9 @@
227 * @method get_vote228 * @method get_vote
228 */229 */
229 get_vote: function() {230 get_vote: function() {
231 if (this.review_fields.hasClass('unseen')) {
232 return null;
233 }
230 var selected_idx = this.vote_input.get('selectedIndex');234 var selected_idx = this.vote_input.get('selectedIndex');
231 var selected = this.vote_input.get('options').item(selected_idx);235 var selected = this.vote_input.get('options').item(selected_idx);
232 if (selected.get('value') === ''){236 if (selected.get('value') === ''){
@@ -361,7 +365,6 @@
361 },365 },
362 renderUI: function() {366 renderUI: function() {
363 CodeReviewComment.superclass.renderUI.apply(this);367 CodeReviewComment.superclass.renderUI.apply(this);
364 Y.one('#inline-add-comment').setStyle('display', 'block');
365 },368 },
366 /**369 /**
367 * Implementation of Widget.bindUI: Bind events to methods.370 * Implementation of Widget.bindUI: Bind events to methods.
@@ -374,8 +377,6 @@
374 bindUI: function() {377 bindUI: function() {
375 CodeReviewComment.superclass.bindUI.apply(this);378 CodeReviewComment.superclass.bindUI.apply(this);
376 this.vote_input.on('mouseup', bind(this.syncUI, this));379 this.vote_input.on('mouseup', bind(this.syncUI, this));
377 this.review_type.on('keyup', bind(this.syncUI, this));
378 this.review_type.on('mouseup', bind(this.syncUI, this));
379 Y.all('a.menu-link-reply').on('click', bind(this.reply_clicked, this));380 Y.all('a.menu-link-reply').on('click', bind(this.reply_clicked, this));
380 },381 },
381 /**382 /**
@@ -388,8 +389,6 @@
388 */389 */
389 syncUI: function() {390 syncUI: function() {
390 CodeReviewComment.superclass.syncUI.apply(this);391 CodeReviewComment.superclass.syncUI.apply(this);
391 var review_type_disabled = (this.get_vote() === null);
392 this.review_type.set('disabled', review_type_disabled);
393 }392 }
394});393});
395Y.lp.CodeReviewComment = CodeReviewComment;394Y.lp.CodeReviewComment = CodeReviewComment;
396395
=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py 2009-10-29 23:51:08 +0000
+++ lib/lp/code/browser/codereviewcomment.py 2009-12-09 08:04:13 +0000
@@ -251,10 +251,11 @@
251 @action('Save Comment', name='add')251 @action('Save Comment', name='add')
252 def add_action(self, action, data):252 def add_action(self, action, data):
253 """Create the comment..."""253 """Create the comment..."""
254 vote = data.get('vote')
255 review_type = data.get('review_type')
254 comment = self.branch_merge_proposal.createComment(256 comment = self.branch_merge_proposal.createComment(
255 self.user, subject=None, content=data['comment'],257 self.user, subject=None, content=data['comment'],
256 parent=self.reply_to, vote=data['vote'],258 parent=self.reply_to, vote=vote, review_type=review_type)
257 review_type=data['review_type'])
258259
259 @property260 @property
260 def next_url(self):261 def next_url(self):
261262
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2009-11-26 23:36:50 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2009-12-09 08:04:13 +0000
@@ -21,6 +21,31 @@
21 #commit-message, #edit-commit-message {21 #commit-message, #edit-commit-message {
22 margin: 1em 0 0 0;22 margin: 1em 0 0 0;
23 }23 }
24 #add-review, #add-comment {
25 margin-bottom: 0.5em;
26 }
27 #add-comment-form {
28 max-width:60em;
29 padding-bottom: 3em;
30 }
31 #add-comment-form textarea{
32 width: 100%;
33 max-width: inherit;
34 }
35 #add-comment-form .actions {
36 float:right;
37 }
38 #add-comment-review-fields {
39 margin-bottom: 0.5em;
40 }
41 #add-comment-review-fields div {
42 display: inline;
43 }
44 #add-comment span, #add-review span {
45 color: black;
46 font-size: 90%;
47 }
48
24 /* A page-specific fix for inline text are editing to line up box. */49 /* A page-specific fix for inline text are editing to line up box. */
25 #edit-commit-message .yui-ieditor-input { top: 0; }50 #edit-commit-message .yui-ieditor-input { top: 0; }
26 </style>51 </style>
@@ -92,7 +117,13 @@
92 </div>117 </div>
93118
94 <div class="yui-g">119 <div class="yui-g">
95 <div tal:define="link menu/add_comment"120 <tal:not-logged-in condition="not: view/user">
121 <div align="center" id="add-comment-login-first">
122 To post a comment you must <a href="+login">log in</a>.
123 </div>
124 </tal:not-logged-in>
125
126 <div tal:define="link context/menu:context/add_comment"
96 tal:condition="link/enabled"127 tal:condition="link/enabled"
97 tal:content="structure link/render">128 tal:content="structure link/render">
98 Add comment129 Add comment
@@ -101,20 +132,38 @@
101 <div id="conversation"132 <div id="conversation"
102 tal:content="structure view/conversation/@@+render"/>133 tal:content="structure view/conversation/@@+render"/>
103 </div>134 </div>
104 <!-- Hide inline commenting if YUI isn't used. -->135
105 <div id="inline-add-comment" style="display: none">136 <tal:logged-in condition="view/user">
106 <tal:comment replace="structure context/@@+comment/++form++" />137 <div tal:define="comment_form nocall:context/@@+comment;
107 <div class="actions" id="launchpad-form-actions">138 dummy comment_form/initialize">
108 <input type="submit" id="field.actions.add" name="field.actions.add" value="Save Comment" class="button" />139 <div id="add-comment" class="unseen">
109 </div>140 <h2>Add comment</h2>
110 </div>141 <a href="#" class="js-action">
111142 <img class="collapseIcon" src="/@@/treeCollapsed"/> show review fields
112 <script type="text/javascript">143 </a>
113 LPS.use('lp.comment', function(Y) {144 </div>
114 var comment = new Y.lp.CodeReviewComment();145 <div id="add-review">
115 comment.render();146 <h2>Add review</h2>
116 })147 <a href="#" class="js-action unseen">
117 </script>148 <img class="collapseIcon" src="/@@/treeExpanded"/> hide review fields
149 </a>
150 </div>
151 <form action="+comment"
152 method="post"
153 enctype="multipart/form-data"
154 accept-charset="UTF-8"
155 id="add-comment-form">
156 <div id="add-comment-review-fields">
157 Review: <tal:review replace="structure comment_form/widgets/vote"/>
158 Review type: <tal:review replace="structure comment_form/widgets/review_type"/>
159 </div>
160 <tal:comment-input replace="structure comment_form/widgets/comment"/>
161 <div class="actions"
162 tal:content="structure comment_form/actions/field.actions.add/render" />
163 </form>
164 </div>
165 </tal:logged-in>
166
118 <div class="yui-g">167 <div class="yui-g">
119 <div class="yui-u first">168 <div class="yui-u first">
120 <div id="source-revisions"169 <div id="source-revisions"
@@ -157,22 +206,28 @@
157<tal:script206<tal:script
158 replace="structure207 replace="structure
159 string:&lt;script id='codereview-script' type='text/javascript'&gt;" />208 string:&lt;script id='codereview-script' type='text/javascript'&gt;" />
160 conf = <tal:status-config replace="view/status_config" />209 conf = <tal:status-config replace="view/status_config" />;
161 <!--210 <!--
162 LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal',211 LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal', 'lp.comment',
163 function(Y) {212 function(Y) {
164213
165214 Y.on('load', function() {
166 if(Y.UA.ie) {215 var logged_in = LP.client.links['me'] !== undefined;
167 return;216
168 }217 if (logged_in) {
169218 var comment = new Y.lp.CodeReviewComment();
170 Y.on('domready', function() {219 comment.render();
171 Y.code.codereview.connect_links();220
172 Y.code.branchmergeproposal.connect_status(conf);221 if(Y.UA.ie) {
173 });222 return;
174223 }
175 (new Y.codereview.NumberToggle()).render();224
225 Y.code.codereview.connect_links(comment);
226 Y.code.branchmergeproposal.connect_status(conf);
227 }
228 (new Y.codereview.NumberToggle()).render();
229 }, window);
230
176231
177 });232 });
178 -->233 -->