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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide resolved comments in changes tree; Indicate resolved comments on margin #2253

Merged
merged 16 commits into from Mar 26, 2019

Conversation

@jcansdale
Copy link
Collaborator

commented Mar 1, 2019

Previously pull request comments would be implicitly resolved when when the text near to the comment was changed. GitHub now supports comments being explicitly resolved by a user and comments becoming implicitly outdated when the text near to the comment is changed.

This pull request surfaces the IsResolved status of a review comment threads and hides resolved comments from the changes tree (in a similar way to how outdated comments are hidden). This makes it easy to find comments that still need to be addressed when responding to a pull request review.

What this PR does

  • Use pullRequest/reviewThreads to populate the IsResolved status of a pull request comment threads
  • When using GitHub Enterprise fall back to using original model (reviewThreads might not be available)
  • Hide resolved comments from the changes tree
  • Show different in-line comment glyph for resolved comments (an empty circle)
  • Show banner to clearly indicate when a thread has been resolved

To do

  • Support fallback for older versions of GitHub Enterprise
  • Indicate outdated in-line comments in glyph
    image
  • Show banner on resolved comment threads
    image

What this PR doesn't do

  • Probe GitHub Enterprise schema to see if we could fetch resolved comment information
  • Surface comments that are outdated but unresolved
  • Allow resolving of comments using inline comment view

How to test

  1. Open https://github.com/grokys/PullRequestSandbox in Visual Studio
  2. Open grokys/PullRequestSandbox#58 in GitHub pane
  3. Check that only 1 comment appears on Program.cs
    image
  4. Click on 馃挰 1 comment
  5. Check Program.cs appears in diff view and comment is visible
  6. Check that empty circle glyph appears below indicating a resolved comment
  7. Click on the resolved comment glyph
  8. Check that The conversation was marked as resolved banner appears above comment

Notes

Here is what is looks like in the Dark theme

image

Related #2245

jcansdale added some commits Mar 1, 2019

Update GraphQL query to use ReviewThreads
Use pullRequest/reviewThreads instead of pullRequest/review/comments.
Add downlevel support for GitHub Enterprise
Only use reviewThreads/isResolved on github.com where they are guaranteed to be available.

@jcansdale jcansdale force-pushed the fixes/2245-comment-thread-resolved-status branch from e2c2462 to 8e4a5a9 Mar 4, 2019

jcansdale added some commits Mar 4, 2019

Show warning on comment thread resolved
Show, "This conversation was marked as resolved" on peek view when conversation has been marked as resolved.

@jcansdale jcansdale force-pushed the fixes/2245-comment-thread-resolved-status branch from 91b62a2 to a489f5d Mar 5, 2019

@jcansdale jcansdale changed the title Show resolved status for pull request review comments Hide resolved comments from changes tree and indicate resolved comments on margin Mar 5, 2019

@jcansdale jcansdale marked this pull request as ready for review Mar 5, 2019

@jcansdale jcansdale requested review from grokys, donokuda and StanleyGoldman Mar 5, 2019

@donokuda

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

I went and did A Thing replacing the info icon with the one built into visual studio. The result is a look and feel closer to a native infobar:

Blue Theme!
screen shot 2019-03-05 at 3 47 52 pm

Dark Theme!
screen shot 2019-03-05 at 3 47 14 pm

At first I was concerned about how much it popped out in the Dark Theme even though I was using the background color that we get from Visual Studio. However, I think it's okay for the following reasons:

  1. I tend to look over this information when the background was less intrusive.
  2. It's what Microsoft does. 馃槅

We've diverged a bit from what I've prototyped but I think that's fine given that my prototype relied heavily on the interaction of collapsing resolved comments.

@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 6, 2019

@donokuda,

At first I was concerned about how much it popped out in the Dark Theme even though I was using the background color that we get from Visual Studio. However, I think it's okay for the following reasons:

  1. I tend to look over this information when the background was less intrusive.

I think this is a good call. 馃憤 We're trying to communicate that someone stuck a fork in this comment thread, you're probably done. Making this pop out is a good thing!

jcansdale added some commits Mar 19, 2019

jcansdale and others added some commits Mar 20, 2019

@StanleyGoldman StanleyGoldman merged commit d0c5fff into master Mar 26, 2019

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
github.VisualStudio Build #20190326.4 succeeded
Details

@StanleyGoldman StanleyGoldman deleted the fixes/2245-comment-thread-resolved-status branch Mar 26, 2019

@StanleyGoldman StanleyGoldman changed the title Hide resolved comments from changes tree and indicate resolved comments on margin Hide resolved comments in changes tree; Indicate resolved comments on margin Apr 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.