Adding the ability to reply to a comment from the diff viewer page
Review Request #1276 — Created Nov. 23, 2009 and discarded
Information | |
---|---|
smorley | |
Review Board | |
Reviewers | |
reviewboard | |
This is a feature enhancement that enables users to reply to comments directly from the diff viewer page.
-
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.
-
-
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.
-
-
-
reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1) Can you add a comment block above this describing the purpose of the function?
-
-
reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1) This should be styled like: return { value: this.text, id: this.replyToCommentId, ... }
-
-
-
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.
-
reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1) We shouldn't duplicate functionality from ReviewReply. Any code needing ot call this function or discardReply should just use a ReviewReply instance.
-
reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1) This doesn't make any sense to me. It shouldn't really be here.
-
-
reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1) Blank line before the call to addReplyCommentBanner, but not after.
-
reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1) Not much point in saying that it's an anonymous function.
-
-
-
reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1) Formatted incorrectly. Should be: gReplyCommentsBanner .slideUp() .find(...) .slideUp();
-
-
-
-
reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1) 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).
-
-
-
reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1) /* * Multiline comments * should look like this. */
-
-
reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1) 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.
-
reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1) This should be: "user": { "username": "", "name": gUserFullName },
-
-
reviewboard/htdocs/media/rb/js/diffviewer.js (Diff revision 1) Alignment issues. Also, blank line after this declaration.
-
-
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.
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) Blank line before this block of declarations and after.
-
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")
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1) This is a docstring. We should be using a comment. Also, no blank line after this.
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1) I don't understand why we're doing this change.
-
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 1) Should be comments, not a doc string.
-
-
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.
-