Adding the ability to reply to a comment from the diff viewer page

Review Request #1276 — Created Nov. 23, 2009 and discarded

Information

Review Board

Reviewers

This is a feature enhancement that enables users to reply to comments directly from the diff viewer page.

 
chipx86
  1. Thanks for the change. I'll review this probably sometime this week (Thanksgiving week is busy). Can you provide screenshots of the process?
  2. 
      
SM
Review request changed

Change Summary:

Uploading screen shots of the process

Screenshots:

-Enter reply to comment
-Save -> Publish/Discard Banner
-After clicking a reply button
chipx86
  1. So lots of stylistic changes to be made. I'd encourage you to be very careful about making sure the code looks just like the existing code.
    
    Along with this, I want to get a better idea about the UI. Replying to comments in the diff viewer is okay if we can get the UI right, but I don't know about the publish banner. What happens if you have two replies to two different people? We certainly wouldn't want more than one banner, but only one banner doesn't make sense either.
    
    How does the banner look when you also have a banner up for a draft review request?
    
    If we had one banner that basically said you have one or more replies (across multiple reviews) with a publish button that published all of them, that might be fine.
    
    However, doing this on a per-comment basis is not what we want here, in terms of UI and code. A lot of the new code adds to DiffComment and treats it like a mini ReviewReply object, when really we should be operating on either a single or multiple ReviewReply objects.
  2. Space before the {
  3. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    These should be in alphabetical order.
    
    How much of these rules are reused from other banners? We should consolidate as much as possible.
  4. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
     
     
     
    Same here.
  5. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
     
     
     
     
    Two blank lines here.
  6. Can you add a comment block above this describing the purpose of the function?
  7. Spaces after "if" and before "{"
  8. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
     
     
     
     
    This should be styled like:
    
    return {
        value: this.text,
        id: this.replyToCommentId,
        ...
    }
  9. } else {
  10. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
     
     
     
    Same comment here about the style.
  11. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
     
     
    No blank line before the }, but you should have one before publishReply. Each function should be separated by a blank line.
  12. We shouldn't duplicate functionality from ReviewReply. Any code needing ot call this function or discardReply should just use a ReviewReply instance.
  13. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
     
     
     
     
     
    This doesn't make any sense to me. It shouldn't really be here.
  14. Space after "if" and before {
  15. reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1)
     
     
     
     
     
    Blank line before the call to addReplyCommentBanner, but not after.
  16. Not much point in saying that it's an anonymous function.
  17. Blank line between these.
  18. { on the same line as the if.
  19. reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1)
     
     
     
     
    Formatted incorrectly. Should be:
    
    gReplyCommentsBanner
        .slideUp()
        .find(...)
        .slideUp();
  20. Blank line between these.
  21. Blank line between these.
  22. reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1)
     
     
     
     
    Formatting issues.
  23. Since the DiffComment won't contain the publish functions, this won't really work. This should instead listen to this event on the comment's parent Review or ReviewReply object (which will need to be modified to emit this signal).
  24. Alignment looks wrong.
  25. Blank line between these.
  26. /*
     * Multiline comments
     * should look like this.
     */
  27. Spaces around the "-"
  28. I get what we're doing here, but it's sort of ugly. Can you make a comment with a TODO saying that this needs to be cleaned up to not need to create this JSON structure?
    
    Ideally, we'd just query for new comments from the server, I think, or deal with the DiffComment structure in the comments array. That's a larger change, though.
  29. This should be:
    
    "user": {
        "username": "",
        "name": gUserFullName
    },
  30. Space on both sides of the "+".
  31. Alignment issues. Also, blank line after this declaration.
  32. Space on both sides of the "+"
  33. reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1)
     
     
     
     
     
    Blank line after the variable declaration.
    
    Is this to fix a bug unrelated to this change? If so, we should split it off into a separate change.
  34. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)
     
     
     
     
    Blank line before this block of declarations and after.
  35. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
    I'd much prefer:
    
    I don't see why we're setting href to that. It should be "#"
    
    Also, I'd prefer setting href and the text with .attr("href", "#") and .text("Reply")
  36. Blank line before the for loop.
  37. This is a docstring. We should be using a comment. Also, no blank line after this.
  38. I don't understand why we're doing this change.
  39. Blank line between these.
  40. Excess whitespace.
  41. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
     
     
     
     
     
    Should be comments, not a doc string.
  42. "attributes"
  43. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Most of this is duplicate code. We need to condense it. Factor out similar code into utility functions.
    
    Actually, though, it seems we're just looping thorugh all comments over and over. The existing outer loop already loops over every comment on that diff, so looping over a subset of them on each comment is unnecessary. We should just make use of the existing checks for the "public" flag and such.
  44. Indentation should be 1 space.
  45. 
      
david
  1. 
      
  2. Can you maybe highlight the comment you're replying to?
  3. 
      
Loading...