Page MenuHomePhabricator

Show more context for inline comments in Differential timeline
Closed, DuplicatePublic

Description

This came up when trying to get the whole company use Phabricator.

In GitHub, each inline comment is shown with its context:

And you can interact with it by reply the comment.

In Phabricator, we only show the comment text with a line number. However, this can be inconvenient when a diff is updated; the user has to click on the line number and open a new tab to see a previous comment.

It would be really nice that one doesn't have to open a new tab to see the context of comments in previous diffs.

Also -- if the comment is for a block of code that is not changed between two diffs, probably make sense to also show it in the latest diff?

Related Objects

Event Timeline

sweetcircle assigned this task to epriestley.
sweetcircle raised the priority of this task from to Needs Triage.
sweetcircle updated the task description. (Show Details)
sweetcircle added a subscriber: sweetcircle.

This UI approach seems to have a number of issues.

  • It doesn't scale well (lots of long comments are going to drastically increase page height, perception of where user is in the review process)
  • Differential also is a 2-up display, with code selection spanning multiple-lines sometimes
  • We have an expectation of well thought out, even marked up text
  • Providing a more 'Table of Contents' view is likely more efficient over repeating every code blocks multiple times

On the other hand
I do like the idea of having 'Dialogs' possibly available as a button/icon next to the line numbers. This gives the user the choice to see the entire context (link to the previous diff) or just a snippet (pop a dialog). Would have to get @epriestley's thoughts.

I think we'll probably revisit inlines, but likely not until after T1460 (mostly a product blocker) and T2009 (mostly a technical blocker). Particularly, T1460 will create some new UI that forces us to take another look at some of this stuff.

Also -- if the comment is for a block of code that is not changed between two diffs, probably make sense to also show it in the latest diff?

This isn't possible in the general case (diffs A and B may be against completely different code), and I worry that doing it conditionally will be confusing. Users already find inlines relatively confusing (T3669, maybe T3639), and having them sometimes port forward and sometimes vanish threatens to make this more complicated. I don't think this is impossible, but I do think it's challenging to implement while making it clear to the user when comments will port forward, why they don't when they don't, and where the went.

Thanks @epriestley! I love the idea in T1460 a lot. Also what @chad suggested makes a lot of sense -- make a popup window with some context would be really awesome.

chad added a subscriber: igorgatis.Jul 30 2014, 1:35 AM

◀ Merged tasks: T5743.

chad renamed this task from Show context for inline comments in the comment box to Show more context for inline comments in Differential timeline.Apr 6 2016, 8:37 PM
chad added subscribers: eadler, bradley.
isfs added a subscriber: isfs.Apr 22 2016, 2:49 AM
isfs removed a subscriber: isfs.Apr 22 2016, 3:32 AM
eadler added a project: Restricted Project.Jul 7 2016, 10:50 PM

I'm not particularly sold on putting context in the timeline, particularly because I think we've received about as much feedback about Differential having "too much stuff" as we have about this context being desirable.

However, I think this information is probably useful to add:

  • Is the inline marked done?
  • Checkbox to let you mark the inline done?
  • Link to the inline in whatever view comes out of T8250.
  • Marker if the inline is a reply?
jayvdb added a subscriber: jayvdb.Jul 7 2016, 11:42 PM
epriestley moved this task from Backlog to Inline Infra on the Differential board.Dec 29 2016, 4:26 PM

I'm just going to merge this into T8250. I'm still unsure what's to become of this, but I think however that resolves will cover this.