Add "Reviewers" section to PR details.

[grokys]

Adds a "Reviewers" section to the PR details view. Looks like this:

Clicking on a review doesn't do anything yet, as that part isn't implemented by this PR.

Depends on #1521
Depends on #1492
Part of #1491
Incorporates changes from #1533

[grokys]

@donokuda need to choose a background for pending reviews on the dark theme as it's currently unreadable. Key is GitHubPendingReviewBackground.

[donokuda]

Updated the dark theme background color for pending reviews:

[donokuda]

I checked out this PR and it looks good to me. There's a couple elements that I plan on reordering, but I can do that in a separate pull request after this lands into the master PR review branch.

I also updated the dark theme background color for pending reviews in cad6463

[meaghanlewis]

@grokys I have a question about this. The Reviewers section only appears to show those reviewers who have already done a review (approved, requested changes, commented).

Take for example, PR 1521. In VS I only see shana's listed as a Reviewer because she has already submitted a review.

But on dotcom, there are other reviewers added who have not yet taken any action on this PR.

I'm just wondering if it's supposed to work like this? Is it possible to show those pending reviewers as well? I feel like it would be helpful, especially if I want to see which PR's I have been added as a reviewer for.

[meaghanlewis]

I don't think this comment is 100% related to this pull request in particular (partly part of #1545 too), but I've noticed that the comment counts shown under each of the Reviewers isn't correct. I think it usually always shows 1 file comments or 0 file comments.

For example, PR 970 shows 1 comment each for shana and jcansdale, but looking at the PR there have been many more comments (even though they are outdated by now- maybe that has something to do with it?)

Note: 1 other interesting thing about this is that jcansdale is listed as a reviewer, but shouldn't be able to be a reviewer on his own PR.

[grokys]

@grokys I have a question about this. The Reviewers section only appears to show those reviewers who have already done a review (approved, requested changes, commented).

Yeah, that's actually a different feature that's not implemented here, namely "requested reviewers". We don't show requested reviewers yet, just reviewers who have already added a review. We will add this feature later I think.

For example, PR 970 shows 1 comment each for shana and jcansdale, but looking at the PR there have been many more comments (even though they are outdated by now- maybe that has something to do with it?)

This looks right to me (at least technically): if I look at the conversation I see:

You can see here that there are a string of reviews with 1 comment. This is because when you leave a comment on a line in a PR with the "Add single comment" button, GitHub creates an implicit review for this comment. We may want to change that in future and merge multiple "commented" reviews into one, but at the moment I think this is working as expected according to the way GitHub functions.

Regarding the review from jcansdale - a user can comment on their own PR, they just can't approve or request changes. The review showing up in your screenshot is Jamie replying to one of shana's review comments, which again creates an implicit review.

[jcansdale]

@grokys, @donokuda,

Could we show the reviewers section in a more condensed format?

For example:

Could the Commented | 1 file comments be displayed to the right of grokys?

This would make it take up less vertical real estate, similar to on .COM:

This space saving would be particularly important when someone has around 10 reviewers (and up)!
See: https://gist.github.com/terrajobst/5de174c777d604cad45f70cd14a456f6

More specifically from @terrajobst:
https://twitter.com/terrajobst/status/974022300445040640
We don't want to make it worse than on .COM! 😉

[meaghanlewis]

You can see here that there are a string of reviews with 1 comment. This is because when you leave a comment on a line in a PR with the "Add single comment" button, GitHub creates an implicit review for this comment. We may want to change that in future and merge multiple "commented" reviews into one, but at the moment I think this is working as expected according to the way GitHub functions.

Sure, I can see what you're saying @grokys but I'll play around with this more (outside of this PR 😄) because it's still confusing me. I interpreted each of shana's comments as separate file comments and would then expect to see 4 file comments.

Regarding the review from jcansdale - a user can comment on their own PR, they just can't approve or request changes. The review showing up in your screenshot is Jamie replying to one of shana's review comments, which again creates an implicit review.

Yes, a user can comment on their own PR but dotcom will never shows as a reviewer of their own PR under Reviewers explicitly. So I'm just wondering if the goal for the reviewers section is to behave like dotcom behaves or not.

[donokuda]

Could the Commented | 1 file comments be displayed to the right of grokys?

I'm 👍for this. I think I tried to give it a shot, but wasn't sure how to bind both the author and review status the GitHubActionLink component

VisualStudio/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestReviewSummaryView.xaml

Lines 62 to 66 in cbb529a

	<ui:GitHubActionLink Grid.Column="1" FontWeight="Bold" Margin="2 1 2 0"
	Command="{Binding Path=DataContext.ShowReview, RelativeSource={RelativeSource Mode=FindAncestor,AncestorType={x:Type local:PullRequestDetailView}}}"
	CommandParameter="{Binding}"
	Content="{Binding User.Login}"
	Foreground="{DynamicResource GitHubVsToolWindowText}"/>

VisualStudio/src/GitHub.VisualStudio/Views/GitHubPane/PullRequestReviewSummaryView.xaml

Lines 80 to 86 in cbb529a

	<TextBlock Grid.ColumnSpan="3" Grid.Row="1"
	Margin="1 0"
	Foreground="{DynamicResource GitHubVsGrayText}">
	<Run Text="{Binding StateDisplay, Mode=OneWay}"/>
	<Run>|</Run>
	<Run Text="{Binding FileCommentCount, Mode=OneWay, StringFormat={}{0} file comments}"/>
	</TextBlock>

[donokuda]

I interpreted each of shana's comments as separate file comments and would then expect to see 4 file comments.

Hmm, I wonder if we should display the total number of comments (for what it's worth, once someone has reviewed or commented on a PR, then they will always appear in the list.)

Other wise, we could clarify that the number of comments relates to the most recent review.

[jcansdale]

@donokuda,

Here's an example using the more condensed format:

From: dotnet/designs#33

I just noticed that on .COM, clicking on the ✔️ or 💬 navigates to the comment and clicking on the user navigates to the user's profile. Now that this looks pretty much identical to .COM, keeping this consistent is maybe important.

Ordering the comments starting with most recent might make this a more manageable (given that you can't dismiss comments, if I've understood correctly).

[jcansdale]

@donokuda,

With this format the ✔️ or 💬 can end up a long way from the username/avitar. For example:

Could this be flowed with as many as fit, something along the lines of:

That way the avatar, username and response would be grouped as a single clickable element. We wouldn't need to worry about being consistent with .COM and only make the ✔️ or 💬 clickable. It would also save a bunch of space when there are lots of reviewers.

Also, what do you think of moving the Reviewers above the Description? To me it doesn't feel right having is sandwiched between the Description and Changes tree.

[donokuda]

I just noticed that on .COM, clicking on the ✔️ or 💬 navigates to the comment and clicking on the user navigates to the user's profile. Now that this looks pretty much identical to .COM, keeping this consistent is maybe important.

I think it's okay to keep the existing functionality of navigating to the user's reviews than going to their profile. I'm concerned that matching dotcom's functionality here will make it less clear how to go to the reviews.

Ordering the comments starting with most recent might make this a more manageable (given that you can't dismiss comments, if I've understood correctly).

I think I'm fine with the ordering as-is. If we want to order by most recent here, we should surface more information so that this is clear. That said, there's an opportunity for us to understand how developers determine which set of reviews to address first (e.g., based on review status, number of file comments, date the review was left.)

[donokuda]

That way the avatar, username and response would be grouped as a single clickable element. We wouldn't need to worry about being consistent with .COM and only make the ✔️ or 💬 clickable. It would also save a bunch of space when there are lots of reviewers.

We could explore this. Ideally, I'd like the status icons to visually align with each other so that it's easier to scan and spot which reviewers are requesting changes. In your example, they do not because the usernames vary in length. To align them, we could consider moving the icons to the left or combining them somehow with the avatar.

We can explore this more post-beta.

[jcansdale]

@donokuda,

I think it's okay to keep the existing functionality of navigating to the user's reviews than going to their profile. I'm concerned that matching dotcom's functionality here will make it less clear how to go to the reviews.

Could we make it so you can click on either the avatar or the ✔️ / 💬? I must be used to .com because I've found myself heading for the ✔️ / 💬.

We could explore this. Ideally, I'd like the status icons to visually align with each other so that it's easier to scan and spot which reviewers are requesting changes. In your example, they do not because the usernames vary in length. To align them, we could consider moving the icons to the left or combining them somehow with the avatar.

Yup, my example looks kind of messy. We could make them a fixed width so they always line up.

Dogfooding this, being able to see the x file comments is kind of useful. I wonder if that could be incorporated in a more condensed way?

[donokuda]

Could we make it so you can click on either the avatar or the ✔️ / 💬? I must be used to .com because I've found myself heading for the ✔️ / 💬.

I'm 👍for that. I'll defer to @grokys on getting that in since I'm not exactly sure how to do it myself (and don't want to push more code to this branch and introduce cascading merge conflicts.)

EDIT:

Dogfooding this, being able to see the x file comments is kind of useful. I wonder if that could be incorporated in a more condensed way?

Definitely! 😄I wanted to remove that for now because it wasn't clear that the number of comments was for last review and not total comments. However, I think what would be really useful is tracking how many comments still needs to be addressed (and maybe across a number of files.) We should consider researching more into this.

[drguthals]

I'd like to capture the comments from others in a separate issue for the next iteration. Otherwise LGTM!

[jcansdale]

Comments captured elsewhere. LGTM!