[WIP] Expand the action-feed boxes for replies

Review Request #3272 — Created Aug. 11, 2012 and discarded

Information

Review Board

Reviewers

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

 
Description From Last Updated

This blank line isn't needed.

chipx86chipx86

These should be witihn the .action_feed or whatever the container is. Otherwise, it's just too generic.

chipx86chipx86

This can be one return statement.

chipx86chipx86

The ) shouldn't be on their own lines. Same comments for others in this file.

chipx86chipx86

The variables need to be indented. We call collapse_button.parent() more than once, so let's cache that into a variable first.

chipx86chipx86

Can be one statement.

chipx86chipx86

Can you do one per line? It'll be easier to read.

chipx86chipx86

One per line.

chipx86chipx86

Initialize it when defining it.

chipx86chipx86

I think this would be nicer as: get_reply_text(comment).appendTo( reply_comments.append($('') ...) Same comments below.

chipx86chipx86

Blank line between these.

chipx86chipx86

Alignment issue.

chipx86chipx86

Alignment problems.

chipx86chipx86

This looks wrong.

chipx86chipx86

Alignment issues.

chipx86chipx86

===

chipx86chipx86

Not sure we want to remove this, since it's a core part of the data on the action. Why's it …

chipx86chipx86

What's this being used for? I'd prefer we call this "replied_to". This would require a serialize_replied_to function.

chipx86chipx86

I'm hesitant to say "comment" here. How about "The review that this is replying to."

chipx86chipx86
chipx86
  1. 
      
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    Show all issues
    This blank line isn't needed.
  3. reviewboard/static/rb/css/action-feed.less (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    These should be witihn the .action_feed or whatever the container is. Otherwise, it's just too generic.
  4. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    This can be one return statement.
  5. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
     
    Show all issues
    The ) shouldn't be on their own lines.
    
    Same comments for others in this file.
  6. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
     
    Show all issues
    The variables need to be indented.
    
    We call collapse_button.parent() more than once, so let's cache that into a variable first.
  7. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    Can be one statement.
  8. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
    Show all issues
    Can you do one per line? It'll be easier to read.
  9. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
    Show all issues
    One per line.
  10. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
    Show all issues
    Initialize it when defining it.
  11. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    I think this would be nicer as:
    
    get_reply_text(comment).appendTo(
        reply_comments.append($('<dt/>')
            ...)
    
    
    Same comments below.
  12. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
    Show all issues
    Blank line between these.
  13. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
    Show all issues
    Alignment issue.
  14. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
    Show all issues
    Alignment problems.
  15. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
     
    Show all issues
    This looks wrong.
    1. I actually forgot to add it in the previous patch.
  16. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
     
    Show all issues
    Alignment issues.
  17. reviewboard/static/rb/js/action-feed.js (Diff revision 1)
     
     
    Show all issues
    ===
  18. reviewboard/webapi/resources.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    Not sure we want to remove this, since it's a core part of the data on the action. Why's it removed?
    1. I removed it since we only need the display_verb and thought there's no need to send duplicate information.
      It's back in now.
  19. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    What's this being used for?
    
    I'd prefer we call this "replied_to". This would require a serialize_replied_to function.
    1. I needed this to display the body_top and body_bottom review comments linked to the current reply (The screenshot, file attachment and diff comments already had a reply_to field).
      I went with 'base_reply_to' since this is how it's referred to in the model and the other resources as well.
  20. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    I'm hesitant to say "comment" here. How about "The review that this is replying to."
  21. 
      
BO
  1. All issues fixed in Review:3292.
  2. 
      
BO
Review request changed
Status:
Discarded