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
1=== modified file 'lib/canonical/launchpad/javascript/code/codereview.js'
2--- lib/canonical/launchpad/javascript/code/codereview.js 2009-11-30 23:51:34 +0000
3+++ lib/canonical/launchpad/javascript/code/codereview.js 2009-12-09 08:04:13 +0000
4@@ -17,13 +17,46 @@
5 /*
6 * Connect all the links to their given actions.
7 */
8-Y.codereview.connect_links = function() {
9+Y.codereview.connect_links = function(comment_widget) {
10
11 var link = Y.one('#request-review');
12 if (link !== null) {
13 link.addClass('js-action');
14 link.on('click', show_request_review_form);
15 }
16+ // Show the link.
17+ Y.one('#add-review a').removeClass('unseen');
18+ var review_fields = Y.one('#add-comment-review-fields');
19+ Y.one('#add-comment a').on('click', function(e) {
20+ e.preventDefault();
21+ Y.one('#add-comment').addClass('unseen');
22+ Y.one('#add-review').removeClass('unseen');
23+ review_fields.removeClass('unseen');
24+ if (review_fields.anim !== undefined)
25+ review_fields.anim.stop();
26+ review_fields.anim = Y.lazr.effects.slide_out(review_fields);
27+ review_fields.anim.run();
28+ comment_widget.syncUI();
29+ });
30+ Y.one('#add-review a').on('click', function(e) {
31+ e.preventDefault();
32+ Y.one('#add-review').addClass('unseen');
33+ Y.one('#add-comment').removeClass('unseen');
34+
35+ if (review_fields.anim !== undefined)
36+ review_fields.anim.stop();
37+ review_fields.anim = Y.lazr.effects.slide_in(review_fields);
38+ review_fields.anim.on('end', function() {
39+ review_fields.addClass('unseen'); });
40+ review_fields.anim.run();
41+
42+ comment_widget.syncUI();
43+ });
44+ // Remove the empty option.
45+ var review_votes = Y.one('#add-comment-review-fields select');
46+ var empty_option = review_votes.query('option[value=""]');
47+ review_votes.removeChild(empty_option);
48+ // Hook up the commit message.
49 link = Y.one('.menu-link-set_commit_message');
50 if (link !== null) {
51 link.addClass('js-action');
52@@ -221,4 +254,4 @@
53
54 Y.codereview.NumberToggle = NumberToggle;
55
56-}, "0.1", {"requires": ["base", "widget", "lazr.anim", "lazr.formoverlay", "lp.picker"]});
57+ }, "0.1", {"requires": ["base", "widget", "lazr.anim", "lazr.effects", "lazr.formoverlay", "lp.picker"]});
58
59=== modified file 'lib/canonical/launchpad/javascript/lp/comment.js'
60--- lib/canonical/launchpad/javascript/lp/comment.js 2009-11-26 19:54:52 +0000
61+++ lib/canonical/launchpad/javascript/lp/comment.js 2009-12-09 08:04:13 +0000
62@@ -211,6 +211,7 @@
63 initializer: function() {
64 this.vote_input = Y.one('[id="field.vote"]');
65 this.review_type = Y.one('[id="field.review_type"]');
66+ this.review_fields = Y.one('#add-comment-review-fields');
67 this.in_reply_to = null;
68 },
69 /**
70@@ -227,6 +228,9 @@
71 * @method get_vote
72 */
73 get_vote: function() {
74+ if (this.review_fields.hasClass('unseen')) {
75+ return null;
76+ }
77 var selected_idx = this.vote_input.get('selectedIndex');
78 var selected = this.vote_input.get('options').item(selected_idx);
79 if (selected.get('value') === ''){
80@@ -361,7 +365,6 @@
81 },
82 renderUI: function() {
83 CodeReviewComment.superclass.renderUI.apply(this);
84- Y.one('#inline-add-comment').setStyle('display', 'block');
85 },
86 /**
87 * Implementation of Widget.bindUI: Bind events to methods.
88@@ -374,8 +377,6 @@
89 bindUI: function() {
90 CodeReviewComment.superclass.bindUI.apply(this);
91 this.vote_input.on('mouseup', bind(this.syncUI, this));
92- this.review_type.on('keyup', bind(this.syncUI, this));
93- this.review_type.on('mouseup', bind(this.syncUI, this));
94 Y.all('a.menu-link-reply').on('click', bind(this.reply_clicked, this));
95 },
96 /**
97@@ -388,8 +389,6 @@
98 */
99 syncUI: function() {
100 CodeReviewComment.superclass.syncUI.apply(this);
101- var review_type_disabled = (this.get_vote() === null);
102- this.review_type.set('disabled', review_type_disabled);
103 }
104 });
105 Y.lp.CodeReviewComment = CodeReviewComment;
106
107=== modified file 'lib/lp/code/browser/codereviewcomment.py'
108--- lib/lp/code/browser/codereviewcomment.py 2009-10-29 23:51:08 +0000
109+++ lib/lp/code/browser/codereviewcomment.py 2009-12-09 08:04:13 +0000
110@@ -251,10 +251,11 @@
111 @action('Save Comment', name='add')
112 def add_action(self, action, data):
113 """Create the comment..."""
114+ vote = data.get('vote')
115+ review_type = data.get('review_type')
116 comment = self.branch_merge_proposal.createComment(
117 self.user, subject=None, content=data['comment'],
118- parent=self.reply_to, vote=data['vote'],
119- review_type=data['review_type'])
120+ parent=self.reply_to, vote=vote, review_type=review_type)
121
122 @property
123 def next_url(self):
124
125=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
126--- lib/lp/code/templates/branchmergeproposal-index.pt 2009-11-26 23:36:50 +0000
127+++ lib/lp/code/templates/branchmergeproposal-index.pt 2009-12-09 08:04:13 +0000
128@@ -21,6 +21,31 @@
129 #commit-message, #edit-commit-message {
130 margin: 1em 0 0 0;
131 }
132+ #add-review, #add-comment {
133+ margin-bottom: 0.5em;
134+ }
135+ #add-comment-form {
136+ max-width:60em;
137+ padding-bottom: 3em;
138+ }
139+ #add-comment-form textarea{
140+ width: 100%;
141+ max-width: inherit;
142+ }
143+ #add-comment-form .actions {
144+ float:right;
145+ }
146+ #add-comment-review-fields {
147+ margin-bottom: 0.5em;
148+ }
149+ #add-comment-review-fields div {
150+ display: inline;
151+ }
152+ #add-comment span, #add-review span {
153+ color: black;
154+ font-size: 90%;
155+ }
156+
157 /* A page-specific fix for inline text are editing to line up box. */
158 #edit-commit-message .yui-ieditor-input { top: 0; }
159 </style>
160@@ -92,7 +117,13 @@
161 </div>
162
163 <div class="yui-g">
164- <div tal:define="link menu/add_comment"
165+ <tal:not-logged-in condition="not: view/user">
166+ <div align="center" id="add-comment-login-first">
167+ To post a comment you must <a href="+login">log in</a>.
168+ </div>
169+ </tal:not-logged-in>
170+
171+ <div tal:define="link context/menu:context/add_comment"
172 tal:condition="link/enabled"
173 tal:content="structure link/render">
174 Add comment
175@@ -101,20 +132,38 @@
176 <div id="conversation"
177 tal:content="structure view/conversation/@@+render"/>
178 </div>
179- <!-- Hide inline commenting if YUI isn't used. -->
180- <div id="inline-add-comment" style="display: none">
181- <tal:comment replace="structure context/@@+comment/++form++" />
182- <div class="actions" id="launchpad-form-actions">
183- <input type="submit" id="field.actions.add" name="field.actions.add" value="Save Comment" class="button" />
184- </div>
185- </div>
186-
187- <script type="text/javascript">
188- LPS.use('lp.comment', function(Y) {
189- var comment = new Y.lp.CodeReviewComment();
190- comment.render();
191- })
192- </script>
193+
194+ <tal:logged-in condition="view/user">
195+ <div tal:define="comment_form nocall:context/@@+comment;
196+ dummy comment_form/initialize">
197+ <div id="add-comment" class="unseen">
198+ <h2>Add comment</h2>
199+ <a href="#" class="js-action">
200+ <img class="collapseIcon" src="/@@/treeCollapsed"/> show review fields
201+ </a>
202+ </div>
203+ <div id="add-review">
204+ <h2>Add review</h2>
205+ <a href="#" class="js-action unseen">
206+ <img class="collapseIcon" src="/@@/treeExpanded"/> hide review fields
207+ </a>
208+ </div>
209+ <form action="+comment"
210+ method="post"
211+ enctype="multipart/form-data"
212+ accept-charset="UTF-8"
213+ id="add-comment-form">
214+ <div id="add-comment-review-fields">
215+ Review: <tal:review replace="structure comment_form/widgets/vote"/>
216+ Review type: <tal:review replace="structure comment_form/widgets/review_type"/>
217+ </div>
218+ <tal:comment-input replace="structure comment_form/widgets/comment"/>
219+ <div class="actions"
220+ tal:content="structure comment_form/actions/field.actions.add/render" />
221+ </form>
222+ </div>
223+ </tal:logged-in>
224+
225 <div class="yui-g">
226 <div class="yui-u first">
227 <div id="source-revisions"
228@@ -157,22 +206,28 @@
229 <tal:script
230 replace="structure
231 string:&lt;script id='codereview-script' type='text/javascript'&gt;" />
232- conf = <tal:status-config replace="view/status_config" />
233+ conf = <tal:status-config replace="view/status_config" />;
234 <!--
235- LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal',
236- function(Y) {
237-
238-
239- if(Y.UA.ie) {
240- return;
241- }
242-
243- Y.on('domready', function() {
244- Y.code.codereview.connect_links();
245- Y.code.branchmergeproposal.connect_status(conf);
246- });
247-
248- (new Y.codereview.NumberToggle()).render();
249+ LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal', 'lp.comment',
250+ function(Y) {
251+
252+ Y.on('load', function() {
253+ var logged_in = LP.client.links['me'] !== undefined;
254+
255+ if (logged_in) {
256+ var comment = new Y.lp.CodeReviewComment();
257+ comment.render();
258+
259+ if(Y.UA.ie) {
260+ return;
261+ }
262+
263+ Y.code.codereview.connect_links(comment);
264+ Y.code.branchmergeproposal.connect_status(conf);
265+ }
266+ (new Y.codereview.NumberToggle()).render();
267+ }, window);
268+
269
270 });
271 -->