Launchpad itself

Merge lp:~thumper/launchpad/comment-ui-tweaks into lp:launchpad

  1. comment-ui-tweaks
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/comment-ui-tweaks
Merge into: lp:launchpad
Diff against target: 161 lines (+66/-31)
3 files modified
lib/canonical/launchpad/javascript/lp/comment.js (+0/-1)
lib/lp/code/browser/codereviewcomment.py (+4/-3)
lib/lp/code/templates/branchmergeproposal-index.pt (+62/-27)
To merge this branch: bzr merge lp:~thumper/launchpad/comment-ui-tweaks
Reviewer Review Type Date Requested Status
Jonathan Lange (community) Approve
Michael Nelson (community) ui Approve
Review via email: mp+15920@code.launchpad.net

Commit message

Change the appearance of the comment form on the merge proposal page, and make it work without javascript.

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

This branch just reformats the comment field somewhat.

Revision history for this message
Tim Penhey (thumper) wrote :

This work was done in a separate pipe to the other branch, and I've taken your suggestion and moved the review fields below the comment field.

Revision history for this message
Michael Nelson (michael.nelson) wrote :

> This work was done in a separate pipe to the other branch, and I've taken your
> suggestion and moved the review fields below the comment field.

Thanks Tim. It looks and works really well - just like the mock-up. But I was keen to hear your thoughts - I mean, I know that this solution suits my workflow for commenting and reviewing, but that's obviously not what's important here. So I'm assuming that you think it's a good general solution, but please say so if not and we can see if Martin has other ideas?

One tiny thing, the 'Review type' field seems to be disabled/enabled only when you click in the comment field, rather than the onchange of the optional review type? (this is nothing to do with your branch, but might be worth fixing - or is there a decision behind that?)

(Original review at https://code.edge.launchpad.net/~thumper/launchpad/hiding-review-fields/+merge/15722 )

review: Approve (ui)
Revision history for this message
Tim Penhey (thumper) wrote :

Thanks for the review Michael.

My thoughts revolve around making the form look nicer. When I saw your mock
up and the suggestion to move the review fields below, it just seemed to make
more sense. I'm happy with the change.

I'll look into the enabling issue now.

Tim

Revision history for this message
Tim Penhey (thumper) wrote :

The enabling and disabling hasn't changed, and it works the same when I tested it locally.

Revision history for this message
Jonathan Lange (jml) wrote :

As discussed on IRC:
  * Space after the colon after max-width in #add-comment-form
  * The phrase "Optional review: Comment only" makes no sense to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/javascript/lp/comment.js'
2--- lib/canonical/launchpad/javascript/lp/comment.js 2009-11-26 19:54:52 +0000
3+++ lib/canonical/launchpad/javascript/lp/comment.js 2009-12-11 05:10:30 +0000
4@@ -361,7 +361,6 @@
5 },
6 renderUI: function() {
7 CodeReviewComment.superclass.renderUI.apply(this);
8- Y.one('#inline-add-comment').setStyle('display', 'block');
9 },
10 /**
11 * Implementation of Widget.bindUI: Bind events to methods.
12
13=== modified file 'lib/lp/code/browser/codereviewcomment.py'
14--- lib/lp/code/browser/codereviewcomment.py 2009-10-29 23:51:08 +0000
15+++ lib/lp/code/browser/codereviewcomment.py 2009-12-11 05:10:30 +0000
16@@ -204,7 +204,7 @@
17
18 class MyDropWidget(DropdownWidget):
19 "Override the default no-value display name to -Select-."
20- _messageNoValue = '-Select-'
21+ _messageNoValue = 'Comment only'
22
23 schema = IEditCodeReviewComment
24
25@@ -251,10 +251,11 @@
26 @action('Save Comment', name='add')
27 def add_action(self, action, data):
28 """Create the comment..."""
29+ vote = data.get('vote')
30+ review_type = data.get('review_type')
31 comment = self.branch_merge_proposal.createComment(
32 self.user, subject=None, content=data['comment'],
33- parent=self.reply_to, vote=data['vote'],
34- review_type=data['review_type'])
35+ parent=self.reply_to, vote=vote, review_type=review_type)
36
37 @property
38 def next_url(self):
39
40=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
41--- lib/lp/code/templates/branchmergeproposal-index.pt 2009-11-26 23:36:50 +0000
42+++ lib/lp/code/templates/branchmergeproposal-index.pt 2009-12-11 05:10:30 +0000
43@@ -21,6 +21,24 @@
44 #commit-message, #edit-commit-message {
45 margin: 1em 0 0 0;
46 }
47+ #add-comment-form {
48+ max-width: 60em;
49+ padding-bottom: 3em;
50+ }
51+ #add-comment-form textarea{
52+ width: 100%;
53+ max-width: inherit;
54+ }
55+ #add-comment-form .actions {
56+ float: right;
57+ margin: 0 -0.5em;
58+ }
59+ #add-comment-review-fields {
60+ margin-top: 1em;
61+ }
62+ #add-comment-review-fields div {
63+ display: inline;
64+ }
65 /* A page-specific fix for inline text are editing to line up box. */
66 #edit-commit-message .yui-ieditor-input { top: 0; }
67 </style>
68@@ -92,6 +110,12 @@
69 </div>
70
71 <div class="yui-g">
72+ <tal:not-logged-in condition="not: view/user">
73+ <div align="center" id="add-comment-login-first">
74+ To post a comment you must <a href="+login">log in</a>.
75+ </div>
76+ </tal:not-logged-in>
77+
78 <div tal:define="link menu/add_comment"
79 tal:condition="link/enabled"
80 tal:content="structure link/render">
81@@ -101,20 +125,27 @@
82 <div id="conversation"
83 tal:content="structure view/conversation/@@+render"/>
84 </div>
85- <!-- Hide inline commenting if YUI isn't used. -->
86- <div id="inline-add-comment" style="display: none">
87- <tal:comment replace="structure context/@@+comment/++form++" />
88- <div class="actions" id="launchpad-form-actions">
89- <input type="submit" id="field.actions.add" name="field.actions.add" value="Save Comment" class="button" />
90- </div>
91- </div>
92-
93- <script type="text/javascript">
94- LPS.use('lp.comment', function(Y) {
95- var comment = new Y.lp.CodeReviewComment();
96- comment.render();
97- })
98- </script>
99+
100+ <tal:logged-in condition="view/user">
101+ <div tal:define="comment_form nocall:context/@@+comment;
102+ dummy comment_form/initialize">
103+ <h2 id="add-comment">Add comment</h2>
104+ <form action="+comment"
105+ method="post"
106+ enctype="multipart/form-data"
107+ accept-charset="UTF-8"
108+ id="add-comment-form">
109+ <tal:comment-input replace="structure comment_form/widgets/comment"/>
110+ <div id="add-comment-review-fields">
111+ Review: <tal:review replace="structure comment_form/widgets/vote"/>
112+ Review type: <tal:review replace="structure comment_form/widgets/review_type"/>
113+ <div class="actions"
114+ tal:content="structure comment_form/actions/field.actions.add/render" />
115+ </div>
116+ </form>
117+ </div>
118+ </tal:logged-in>
119+
120 <div class="yui-g">
121 <div class="yui-u first">
122 <div id="source-revisions"
123@@ -159,21 +190,25 @@
124 string:&lt;script id='codereview-script' type='text/javascript'&gt;" />
125 conf = <tal:status-config replace="view/status_config" />
126 <!--
127- LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal',
128+ LPS.use('io-base', 'code.codereview', 'code.branchmergeproposal', 'lp.comment',
129 function(Y) {
130
131-
132- if(Y.UA.ie) {
133- return;
134- }
135-
136- Y.on('domready', function() {
137- Y.code.codereview.connect_links();
138- Y.code.branchmergeproposal.connect_status(conf);
139- });
140-
141- (new Y.codereview.NumberToggle()).render();
142-
143+ Y.on('load', function() {
144+ var logged_in = LP.client.links['me'] !== undefined;
145+
146+ if (logged_in) {
147+ var comment = new Y.lp.CodeReviewComment();
148+ comment.render();
149+
150+ if(Y.UA.ie) {
151+ return;
152+ }
153+
154+ Y.code.codereview.connect_links();
155+ Y.code.branchmergeproposal.connect_status(conf);
156+ }
157+ (new Y.codereview.NumberToggle()).render();
158+ }, window);
159 });
160 -->
161 <tal:script replace="structure string:&lt;/script&gt;" />