Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@grokys
Copy link
Contributor

@grokys grokys commented Jan 11, 2018

The PR adds an initial spike of the Pull Request Reviews feature.

Existing reviews are displayed in the PR details pane, filtered to the most recent (non-dismissed, non-pending) review for each user:

image

Clicking on the review opens a new pane:

image

Clicking on "Start a review" similarly opens a new pane for authoring a new review:

image

Known Issues

This is an initial spike of the feature, and as such there are limitations and known issues:

  • The GitHub REST API does not support modifying pending reviews. This means:
    • If you've already started a review of the PR on GitHub, don't try to do a review on the same PR here. It will break
    • We have to keep a record of the line comments for the current review locally. These aren't serialized currently, so closing the solution/VS will mean you lose your comments Fixed: now using GraphQL
    • The "Approve" and "Request Changes" actions are available when reviewing your own PRs. These will fail in this case, you can only comment on your own PRs
  • It's not possible to tell which inline comments are part of the review being looked at/authored and which aren't

Moving forward

  • We need to decide whether to continue storing the state of a pending review locally, or move to GraphQL which supports updating a pending review I've gone with GraphQL
  • We need to decide how to display inline comments glyphs for comments that are outside the current review

Depends on #1445

grokys added 20 commits January 8, 2018 09:29
`PullRequestDetailView` was not displaying in the Visual Studio designer - it was showing an error related to `OcticonImage`.

 Not sure why this works when the previous didn't, but seems to fix it.
Display an overview of the current reviews in the PR details pane.
Need to make sure we match the start and end of the string.
We're now implementing diffing and opening files at the view model level in `GitHub.App`. I'm conflicted on whether this is the best place to do this, but it's the easiest way for now.
Just the view so far, doesn't really do much.
Hit a snag here: the REST API doesn't allow us to add comments to pending reviews, so instead of creating a pending review we just store everything in memory for now.

Submitting reviews not yet implemented.
I'm not sure how this was working...
Also move the changed files into a tab control.
Include outdated comments in comment count. Should we do this? Not sure, but this is how we're counting them in the PR details view.
@jcansdale
Copy link
Collaborator

I've been dogfooding this PR the last couple of days.

When returning to a PR, the first thing I want to see is whether there are any new reviews/approvals. With the current layout, you need to scroll down past the PR description to find this information. The concise Reviewers section can easily be missed, particularly when there is a long description and many PR file changes (this PR is a case in point).

I think it would work better if the Reviewers section was at the top:

image

That way is would be easy to see the status of a PR, navigate to the commands and find the Start a review button.

In a similar vein, maybe the View conversation on GitHub should be moved below the Changes (it can also be a pain to find). This would mirror the other view options that appear at the bottom of a PR on .com:

image

Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No scrollbar appears on the comments view when it becomes too tall.

See other PR comment here:
#1415 (comment)

Looking good. 😄

It was part of the unfinished "PR conversation view" feature.
Instead of starting a new review by clicking on the "Start a review" link in the PR details, start a review in the same way as .com by clicking a "Start a review" button in an inline comment view.
grokys added a commit that referenced this pull request Mar 16, 2018
Ported from #1415. Moves the PR details changed files tree into its own view so that it can be shared by the PR reviews view.
grokys added a commit that referenced this pull request Mar 19, 2018
Ported from #1415. Moves the PR details changed files tree into its own view so that it can be shared by the PR reviews view.
@grokys
Copy link
Contributor Author

grokys commented Mar 23, 2018

Superseded by #1491.

@grokys grokys closed this Mar 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants