Page MenuHomePhabricator

Inline comments displayed on the same line are shown in creation order without regard for threading
Closed, ResolvedPublic

Description

Inline comments appearing beneath the same line are always ordered by ID. This makes some sense for single-threaded comments because chronological order is broadly a reasonable order, but is confusing for replies:

Screen Shot 2016-03-10 at 3.57.16 PM.png (631×946 px, 62 KB)

To reproduce:

  • Make multiple comments on a line.
  • Reply to the comments several times, then reply to the replies, etc., producing a rich tapestry of discussion threads.

Comments are displayed in internal ID order, which is better than, say, random, but this makes threading ambiguous and is unquestionably less desirable than showing them in threaded order.

Providing some hints in the display treatment would also make inline threads easier to follow.

Event Timeline

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Mar 11 2016, 12:29 AM

D15459 is a partial fix here. Here's an "after":

Screen Shot 2016-03-10 at 4.33.26 PM.png (676×956 px, 69 KB)

Remaining issues are:

  • When adding comments, the edit dialog is put at the bottom of the group, not in the right place inline. I think this isn't too confusing, and fixing the JS is a little trickier.
  • We've lost the ability to identify a new comment at the end of a subthread. Previously, it would be at the bottom; now, it's in the middle somewhere, according to its threaded position. A better approach might be blending threading and chronological order, so that replies were grouped by top-level comment but all rendered inline by chronology underneath. Given "Done" and "Hide" and the context provided by the comments themselves, this concern may not be much of an issue in practice.
  • The design treatment could probably be improved. I've made a minimal adjustment to make threading more clear, but haven't tried to really surface threading much in the UI.

So I think this is a big step forward, but maybe not a complete solution.

This change is retroactive, and existing threads should display properly after update.

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 7 2016, 6:05 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 7 2016, 6:08 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.May 13 2016, 6:05 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:00 PM

I'm going to fix the editor behavior and then call this done.

Per T10563#221154, D17938 makes "reply" put comments in the right (threading-respecting) place and marks this complete.

We may eventually revisit how we communicate threading relationships in the actual UI, but ordering things sensibly appears to have resolved the bulk of this.