-
-
-
reviewboard/static/rb/css/action-feed.less (Diff revision 1) These should be witihn the .action_feed or whatever the container is. Otherwise, it's just too generic.
-
-
reviewboard/static/rb/js/action-feed.js (Diff revision 1) The ) shouldn't be on their own lines. Same comments for others in this file.
-
reviewboard/static/rb/js/action-feed.js (Diff revision 1) The variables need to be indented. We call collapse_button.parent() more than once, so let's cache that into a variable first.
-
-
reviewboard/static/rb/js/action-feed.js (Diff revision 1) Can you do one per line? It'll be easier to read.
-
-
-
reviewboard/static/rb/js/action-feed.js (Diff revision 1) I think this would be nicer as: get_reply_text(comment).appendTo( reply_comments.append($('<dt/>') ...) Same comments below. -
-
-
-
-
-
-
reviewboard/webapi/resources.py (Diff revision 1) Not sure we want to remove this, since it's a core part of the data on the action. Why's it removed?
-
reviewboard/webapi/resources.py (Diff revision 1) What's this being used for? I'd prefer we call this "replied_to". This would require a serialize_replied_to function.
-
reviewboard/webapi/resources.py (Diff revision 1) I'm hesitant to say "comment" here. How about "The review that this is replying to."
[WIP] Expand the action-feed boxes for replies
Review Request #3272 — Created Aug. 11, 2012 and discarded
The title is self-explanatory, I hope. I'm not particularly happy with the way it looks, so any improvement suggestions will be appreciated. As for the implementation, I added the 'base_reply_to' field to the ReviewReplyResource so I could get the body_top and body_bottom text of the original review. If this is ok, then we could probably drop the get_base_reply_to_field method in the same class. Still TODO: eliminate duplication between the javascript functions for expanding review and reply boxes
Screenshots
| Description | From | Last Updated |
|---|---|---|
|
This blank line isn't needed. |
 chipx86
|
|
|
These should be witihn the .action_feed or whatever the container is. Otherwise, it's just too generic. |
chipx86
|
|
|
This can be one return statement. |
chipx86
|
|
|
The ) shouldn't be on their own lines. Same comments for others in this file. |
chipx86
|
|
|
The variables need to be indented. We call collapse_button.parent() more than once, so let's cache that into a variable first. |
chipx86
|
|
|
Can be one statement. |
chipx86
|
|
|
Can you do one per line? It'll be easier to read. |
chipx86
|
|
|
One per line. |
chipx86
|
|
|
Initialize it when defining it. |
chipx86
|
|
|
I think this would be nicer as: get_reply_text(comment).appendTo( reply_comments.append($('') ...) Same comments below. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Alignment issue. |
chipx86
|
|
|
Alignment problems. |
chipx86
|
|
|
This looks wrong. |
chipx86
|
|
|
Alignment issues. |
chipx86
|
|
|
=== |
chipx86
|
|
|
Not sure we want to remove this, since it's a core part of the data on the action. Why's it … |
chipx86
|
|
|
What's this being used for? I'd prefer we call this "replied_to". This would require a serialize_replied_to function. |
chipx86
|
|
|
I'm hesitant to say "comment" here. How about "The review that this is replying to." |
chipx86
|
