Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start PR Reviews from Inline Comments #1562

Merged
merged 7 commits into from Apr 9, 2018

Conversation

@grokys
Copy link
Contributor

@grokys grokys commented Mar 23, 2018

This PR adds a "Start a review" button to the inline comment peek view. When clicked it will start a pending review, and from this point all inline comments will be added to the pending review until the review is submitted:

2018-03-23_12-29-17

Depends on #1549
Part of #1491

grokys added 5 commits Mar 21, 2018
You can't actually start the review yet though!
Track the current pending review to `PullRequestSession` along with methods to start reviews, post review comments and submit reviews.
@grokys grokys changed the title Integrate PR Reviews into Inline Comments Start PR Reviews from Inline Comments Mar 23, 2018
@grokys grokys mentioned this pull request Mar 23, 2018
17 of 17 tasks complete
@grokys grokys mentioned this pull request Mar 27, 2018
@meaghanlewis
Copy link
Contributor

@meaghanlewis meaghanlewis commented Mar 27, 2018

This PR looks good to me 👍

I mainly followed scenarios that I outlined here

Just a couple things to note:

  1. The error that I get when trying to request changes or leave a comment review when there are no comments is too long and seems out of place.

screen shot 2018-03-27 at 9 24 56 am

For other errors don't we usually have a banner at the top (like 👇 ) ?
error banner

cc @grokys @donokuda

  1. The cancel button doesn't seem to do anything
    cancel doesn t do anything
@donokuda
Copy link
Member

@donokuda donokuda commented Mar 27, 2018

For other errors don't we usually have a banner at the top (like 👇 ) ?

Yup, that's correct. I think the banner up there makes the most sense to show the error.

Starting a review by replying to an existing comment was causing the submitted comment's text to be copied into the placeholder after submission. Add a unit test to check for this and fix it.
@sguthals
Copy link
Contributor

@sguthals sguthals commented Mar 29, 2018

I agree with @meaghanlewis regarding the error message.

I'm also noticing on VS2017 the font color is too dark for the changes list:
screen shot 2018-03-28 at 6 28 30 pm

I also noticed that if you have "" in your comments you get an error:
screen shot 2018-03-28 at 6 31 55 pm

screen shot 2018-03-28 at 6 32 10 pm

I took out the quotes and it worked fine.

The interesting thing is that I started the inline comment, clicked "Start Your Review" and it started it, because I was able to see the "Continue your review", but the comment didn't actually get submitted until I took out the quotes.

And I confirmed only one version of the inline comment was saved:
screen shot 2018-03-28 at 6 34 12 pm

Is that a known issue?

…ature/pr-review-authoring
@grokys grokys mentioned this pull request Mar 29, 2018
8 of 18 tasks complete
@jcansdale jcansdale changed the base branch from master to feature/pr-reviews-master Apr 9, 2018
@jcansdale jcansdale merged commit bb8d412 into feature/pr-reviews-master Apr 9, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@meaghanlewis meaghanlewis added this to Done in 2.4.4 Apr 13, 2018
@meaghanlewis meaghanlewis removed this from Done in 2.4.4 Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.