Adding the ability to reply to a comment from the diff viewer page
Review Request #1276 — Created Nov. 23, 2009 and discarded
This is a feature enhancement that enables users to reply to comments directly from the diff viewer page.
SM
- Change Summary:
-
Uploading screen shots of the process
- Screenshots:
-
Enter reply to commentSave -> Publish/Discard BannerAfter clicking a reply button
-
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.
-
-
These should be in alphabetical order. How much of these rules are reused from other banners? We should consolidate as much as possible.
-
-
-
-
-
-
-
-
No blank line before the }, but you should have one before publishReply. Each function should be separated by a blank line.
-
We shouldn't duplicate functionality from ReviewReply. Any code needing ot call this function or discardReply should just use a ReviewReply instance.
-
-
-
-
-
-
-
-
-
-
-
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).
-
-
-
-
-
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.
-
-
-
-
-
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.
-
-
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")
-
-
-
-
-
-
-
-
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.
-