Page MenuHomePhabricator

It's difficult to know an inline comment didn't post/save in Pholio
Closed, ResolvedPublic

Description

I didn't know inline comments didn't submit on their own, there isn't any visual indication like on Differential. Personally, I think the use case here is different than in Differential and that a full comment submit shouldn't be needed, they should just post/save. I would also argue that maybe Differential should do the same, which solves race-condition peer reviews, but we can discuss that another time.

Event Timeline

The big reason we batch them is: if I leave 30 inlines, should we send 30 emails?

(Less concretely: my inlines often don't make sense without the context of the main comment [and I believe this is common in general]; I often revise inlines heavily during review.)

We should definitely indicate the draft state prominently, though.

You raise a good point for Differential. I don't know if it is perfectly parallel in Pholio. What about some way of letting the user decide if they want to add additional info or just leave an inline comment in Pholio? The problem I'm seeing is with comments living on the mocks as well as inline leads to this disjointed timeline of partial conversations. It just seems more of an issue to me in Pholio vs. Differential. It also might be how the transactions are rending in the Timeline. Inline comments render as individual transactions instead of inside a comment.

If we just want to punt and stick to what Differential does, this task is essentially:

  1. Show a draft state to an inline comment.
  2. Have inline comment appear inside other comment transaction.

Is that correct?

Yeah -- I figure we'll start there and see how it goes.

I would love to 'tag' this with a status of 'launch blocker' or something to let us know to fix it before un-beta. :)

epriestley edited this Maniphest Task.
epriestley triaged this task as Normal priority.Mar 3 2013, 7:41 PM
chad claimed this task.
chad changed the visibility from "All Users" to "Public (No Login Required)".Mar 17 2016, 3:06 AM