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

PR Review Feature Request: review comment changes #1897

Merged
merged 20 commits into from Mar 1, 2019

Conversation

5 participants
@smashwilson
Copy link
Member

smashwilson commented Jan 9, 2019

In our sprint planning meeting today, @vanessayuenn and @kuychaco had a number of ideas for how we could handle:

  • Displaying review summary comments. Putting these in a separate "Reviews" tab within the PullRequestDetailItem feels redundant and "dotcom-y".
  • Displaying review comments within open TextEditors. @vanessayuenn brought up that displaying the review comment content directly in the TextEditor with a block decoration could be disruptive to the editing experience: if there's a huge comment thread in the middle of the function you're trying to edit, you want to be able to read the comments and see the full function's source at once, even if the review comment was left on a line in the middle. Additionally, it would be beneficial to give users a way to see where a specific review is within the review it belongs to, and to navigate quickly among them.

@vanessayuenn, @kuychaco: Please correct me if I've misrepresented what you were going for, or if you've changed your minds about anything since the ideas have had a chance to percolate somewhat 😄

cc @simurai, because this is some of the stuff we'd like to run by you in our design meeting tonight 😇


Prototype: prototypes/1897-reviews branch Use github:open-mock command to see the "Reviews dock" item.

reviews


View rendered docs/feature-requests/003-pull-request-review.md

smashwilson and others added some commits Jan 9, 2019

Document using a "PullRequestReviewsItem" for review navigation.
Co-Authored-By: Vanessa Yuen <vanessayuenn@users.noreply.github.com>
Co-Authored-By: Katrina Uychaco <katrina@github.com>
Describe the review summary panel within the "Files Changed" tab
Co-Authored-By: Vanessa Yuen <vanessayuenn@users.noreply.github.com>
Co-Authored-By: Katrina Uychaco <katrina@github.com>

@smashwilson smashwilson added the rfc label Jan 9, 2019

@smashwilson smashwilson added this to In Progress 🔧 in Sprint : 9 January 2019 - 12 February 2019 : v0.25.0 via automation Jan 9, 2019

@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Add "open" button

> TODO: Add "open" button
![reviews panel](https://user-images.githubusercontent.com/378023/46536010-17ad4780-c8e8-11e8-8338-338bb592efc5.png)
A panel at the bottom of the pane shows the progress for resolved review comments. It also has a "Review Changes" button to create a new review. This panel is persistent throughout all sub-views. It allows creating new reviews no matter where you are. Below examples with the existing sub-views:


This comment was generated by todo based on a TODO comment in 11fb558 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Change "Show review comments" checkbox to an expand/collapse review summaries control

> TODO: Change "Show review comments" checkbox to an expand/collapse review summaries control
![files](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png)
Clicking on the "Expand review summaries" control in the filter bar reveals an inline panel that displays the summary of each review created on this pull request, including the review's author, the review's current state, its summary comment, and a progress bar showing how many of the review comments associated with this review have been marked as resolved.


This comment was generated by todo based on a TODO comment in 11fb558 in #1897. cc @atom.
@todo

This comment was marked as resolved.

Copy link

todo bot commented Jan 9, 2019

Illustrate the "review summary" list panel

> TODO: Illustrate the "review summary" list panel
Clicking the checkbox within each review summary block hides or reveals the review summary comments associated with that review in diff on this tab. Clicking the "Collapse review summaries" control conceals the review summary panel again.
Beneath the review summary panel is the pull request's combined diff. Diffs are editable, but _only_ if the pull request branch is checked out and the local branch history has not diverged incompatibly from the remote branch history.


This comment was generated by todo based on a TODO comment in 11fb558 in #1897. cc @atom.
@todo

This comment was marked as resolved.

Copy link

todo bot commented Jan 9, 2019

Illustration for the PullRequestReviewsItem

> TODO: Illustration for the PullRequestReviewsItem
Reviews are sorted by "urgency," showing reviews that still need to be addressed at the top. Within each group, sorting is done by "newest first".
1. "recommended" changes
2. "commented" changes


This comment was generated by todo based on a TODO comment in 11fb558 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Illustrate the "review comment here" gutter and line decorations

> TODO: Illustrate the "review comment here" gutter and line decorations
Clicking on the gutter icon reveals the `PullRequestReviewsItem` and highlights that review comment as the "current" one, scrolling to it and expanding its review if necessary.
### Context and navigation


This comment was generated by todo based on a TODO comment in 11fb558 in #1897. cc @atom.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 9, 2019

Codecov Report

Merging #1897 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1897      +/-   ##
==========================================
- Coverage   91.05%   91.02%   -0.03%     
==========================================
  Files         185      185              
  Lines       10719    10719              
  Branches     1575     1575              
==========================================
- Hits         9760     9757       -3     
- Misses        959      962       +3
Impacted Files Coverage Δ
lib/atom/gutter.js 89.74% <0%> (-2.57%) ⬇️
lib/atom/decoration.js 84.33% <0%> (-1.21%) ⬇️
lib/git-shell-out-strategy.js 87.52% <0%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91dac9a...51e2f84. Read the comment docs.

@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Add "open" button

> TODO: Add "open" button
![reviews panel](https://user-images.githubusercontent.com/378023/46536010-17ad4780-c8e8-11e8-8338-338bb592efc5.png)
A panel at the bottom of the pane shows the progress for resolved review comments. It also has a "Review Changes" button to create a new review. This panel is persistent throughout all sub-views. It allows creating new reviews no matter where you are. Below examples with the existing sub-views:


This comment was generated by todo based on a TODO comment in 4696ec6 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Change "Show review comments" checkbox to an expand/collapse review summaries control

> TODO: Change "Show review comments" checkbox to an expand/collapse review summaries control
![files](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png)
Clicking on the "Expand review summaries" control in the filter bar reveals an inline panel that displays the summary of each review created on this pull request, including the review's author, the review's current state, its summary comment, and a progress bar showing how many of the review comments associated with this review have been marked as resolved.


This comment was generated by todo based on a TODO comment in 4696ec6 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Illustrate the "review comment here" gutter and line decorations

> TODO: Illustrate the "review comment here" gutter and line decorations
Clicking on the gutter icon reveals the `PullRequestReviewsItem` and highlights that review comment as the "current" one, scrolling to it and expanding its review if necessary.
### Context and navigation


This comment was generated by todo based on a TODO comment in 4696ec6 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Add "open" button

> TODO: Add "open" button
![reviews panel](https://user-images.githubusercontent.com/378023/46536010-17ad4780-c8e8-11e8-8338-338bb592efc5.png)
A panel at the bottom of the pane shows the progress for resolved review comments. It also has a "Review Changes" button to create a new review. This panel is persistent throughout all sub-views. It allows creating new reviews no matter where you are. Below examples with the existing sub-views:


This comment was generated by todo based on a TODO comment in 3aeb59e in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Change "Show review comments" checkbox to an expand/collapse review summaries control

> TODO: Change "Show review comments" checkbox to an expand/collapse review summaries control
![files](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png)
Clicking on the "Expand review summaries" control in the filter bar reveals an inline panel that displays the summary of each review created on this pull request, including the review's author, the review's current state, its summary comment, and a progress bar showing how many of the review comments associated with this review have been marked as resolved.


This comment was generated by todo based on a TODO comment in 3aeb59e in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 9, 2019

Illustrate the "review comment here" gutter and line decorations

> TODO: Illustrate the "review comment here" gutter and line decorations
Clicking on the gutter icon reveals the `PullRequestReviewsItem` and highlights that review comment as the "current" one, scrolling to it and expanding its review if necessary.
### Context and navigation


This comment was generated by todo based on a TODO comment in 3aeb59e in #1897. cc @atom.

simurai added a commit that referenced this pull request Jan 15, 2019

Replace PR list
with progress bars
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Add "open" button

> TODO: Add "open" button
![reviews panel](https://user-images.githubusercontent.com/378023/46536010-17ad4780-c8e8-11e8-8338-338bb592efc5.png)
A panel at the bottom of the pane shows the progress for resolved review comments. It also has a "Review Changes" button to create a new review. This panel is persistent throughout all sub-views. It allows creating new reviews no matter where you are. Below examples with the existing sub-views:


This comment was generated by todo based on a TODO comment in 49d1212 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Change "Show review comments" checkbox to an expand/collapse review summaries control

> TODO: Change "Show review comments" checkbox to an expand/collapse review summaries control
![files](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png)
Clicking on the "Expand review summaries" control in the filter bar reveals an inline panel that displays the summary of each review created on this pull request, including the review's author, the review's current state, its summary comment, and a progress bar showing how many of the review comments associated with this review have been marked as resolved.


This comment was generated by todo based on a TODO comment in 49d1212 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Illustrate the "review comment here" gutter and line decorations

> TODO: Illustrate the "review comment here" gutter and line decorations
Clicking on the gutter icon reveals the `PullRequestReviewsItem` and highlights that review comment as the "current" one, scrolling to it and expanding its review if necessary.
### Context and navigation


This comment was generated by todo based on a TODO comment in 49d1212 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Add "open" button

> TODO: Add "open" button
![reviews panel](https://user-images.githubusercontent.com/378023/46536010-17ad4780-c8e8-11e8-8338-338bb592efc5.png)
A panel at the bottom of the pane shows the progress for resolved review comments. It also has a "Review Changes" button to create a new review. This panel is persistent throughout all sub-views. It allows creating new reviews no matter where you are. Below examples with the existing sub-views:


This comment was generated by todo based on a TODO comment in 6c6a5b6 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Change "Show review comments" checkbox to an expand/collapse review summaries control

> TODO: Change "Show review comments" checkbox to an expand/collapse review summaries control
![files](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png)
Clicking on the "Expand review summaries" control in the filter bar reveals an inline panel that displays the summary of each review created on this pull request, including the review's author, the review's current state, its summary comment, and a progress bar showing how many of the review comments associated with this review have been marked as resolved.


This comment was generated by todo based on a TODO comment in 6c6a5b6 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Illustrate the "review comment here" gutter and line decorations

> TODO: Illustrate the "review comment here" gutter and line decorations
Clicking on the gutter icon reveals the `PullRequestReviewsItem` and highlights that review comment as the "current" one, scrolling to it and expanding its review if necessary.
### Context and navigation


This comment was generated by todo based on a TODO comment in 6c6a5b6 in #1897. cc @atom.
Replace header and footer
Plus remove outdated mockups
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Change "Show review comments" checkbox to an expand/collapse review summaries control

> TODO: Change "Show review comments" checkbox to an expand/collapse review summaries control
![files](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png)
Clicking on the "Expand review summaries" control in the filter bar reveals an inline panel that displays the summary of each review created on this pull request, including the review's author, the review's current state, its summary comment, and a progress bar showing how many of the review comments associated with this review have been marked as resolved.


This comment was generated by todo based on a TODO comment in b72a006 in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Illustrate the "review comment here" gutter and line decorations

> TODO: Illustrate the "review comment here" gutter and line decorations
Clicking on the gutter icon reveals the `PullRequestReviewsItem` and highlights that review comment as the "current" one, scrolling to it and expanding its review if necessary.
### Context and navigation


This comment was generated by todo based on a TODO comment in b72a006 in #1897. cc @atom.

simurai added some commits Jan 17, 2019

@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Illustrate the "review comment here" gutter and line decorations

> TODO: Illustrate the "review comment here" gutter and line decorations
Clicking on the gutter icon reveals the `PullRequestReviewsItem` and highlights that review comment as the "current" one, scrolling to it and expanding its review if necessary.
### Context and navigation


This comment was generated by todo based on a TODO comment in dccb70a in #1897. cc @atom.
@todo

This comment has been minimized.

Copy link

todo bot commented Jan 17, 2019

Illustrate the "review comment here" gutter and line decorations

> TODO: Illustrate the "review comment here" gutter and line decorations
Clicking on the gutter icon reveals the `PullRequestReviewsItem` and highlights that review comment as the "current" one, scrolling to it and expanding its review if necessary.
### Context and navigation


This comment was generated by todo based on a TODO comment in 5ca37fb in #1897. cc @atom.
@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 22, 2019

As an update on our progress here:

  • We'll use the prototypes that @simurai and @vanessayuenn are hashing out in some of our upcoming UXR sessions, to hear how prospective users feel about some of the directions we could take.
  • @vanessayuenn and @kuychaco will assemble a write-up of the design directions that we're exploring, articulating and giving a name to each, so we can explore a discussion of the pros and cons of each, and link it from here.
@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jan 29, 2019

At some point we have to decide how reviews and review comments should be sorted. To get a feel for it, there are two prototypes for the "first level" of sorting:

By reviewer By file
Review comments get shown in the same order as they are made and therefore grouped by reviewer. Review comments and summaries are split into two groups. Review comments are ordered by file and line number.
Branch: vy/prototype-for-uxr Branch: vy/prototype-sorted-by-file
by-reviewer by-file

There are cons and pros for both and it maybe comes down to personal preference, but something I just realized now: When writing a review, the reviewer might talk about the previously made comment. If the comments get sorted by file, they may appear in a different order as they have been written and then it may not make sense if they say something like "same as above". See this example:

How the comments got written Shown in a random order (by file)
as-authored by-file

Therefore I think as a default, showing review comments in the same order how they got written makes the most sense.

@robertrossmann

This comment has been minimized.

Copy link
Contributor

robertrossmann commented Jan 29, 2019

I'd like to suggest how I would personally prefer to see the reviews in Atom.

For me, there are two types of review comments (not sure how they are named here):

  1. The PR review comment - it's the comment you leave when you hit the green "Submit Review" button

I would prefer these comments to be situated in a completely separate group somewhere in the UI, probably on top

  1. The review comment - it's the comment you leave on a line of code which is directly tied to a specific position/code in a file

I would prefer to see these comments grouped by file, sorted by the time they were authored by the reviewer, because it is easier for me to address all comments for a specific file while I have that file open, rather than switch files all the time.

To address the problem with some review comments being shown in weird order, I think the ideal solution would be to sort the files in the same way they are sorted on Github.com. Then the comments can be grouped by file and sorted by time of creation and still they would appear in the same way as they appear on Github.com . This, however, requires that the overall review comments be shown separately, maybe on top of the review comments, in their own section maybe. 🤔

@vanessayuenn

This comment has been minimized.

Copy link
Contributor

vanessayuenn commented Jan 29, 2019

I think the ideal solution would be to sort the files in the same way they are sorted on Github.com.

I like this suggestion a lot!

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jan 30, 2019

I think the ideal solution would be to sort the files in the same way they are sorted on Github.com.

That’s a great suggestion. 👍

I just realized that the vy/prototype-sorted-by-file prototype already is ordered the same as github.com. So I guess the question is: Does it not bother you that there might be review comments from different reviews/reviewers mixed together? On github.com you have both options:

  1. Go to Files changed to read comments by file.
  2. Go to Conversation to read comments by review.

In the "this is a really long name" example, it's nice that the other related comments are following right after so you can address them at the same time.

Too much of an edge case? If so, we could implement "sorted by files" first and maybe add an option to sort by review later.

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jan 30, 2019

Could we implement per-review filtering in vy/prototype-sorted-by-file? Maybe have a checkbox or a toggling eye icon next to each review in the "Reviews" section, then only render review comment threads that include review comments from selected reviews. If you wanted to see the comments from a single review, you could filter it to only that one.

Also of these two options I'm 👍 on vy/prototype-sorted-by-file, btw. I like that it makes review comment threads with comments from different reviews a non-issue, and I like that it groups comment threads in a way that you're likely to naturally resolve them.

@robertrossmann

This comment has been minimized.

Copy link
Contributor

robertrossmann commented Jan 30, 2019

So I guess the question is: Does it not bother you that there might be review comments from different reviews/reviewers mixed together?

I believe you almost always want to see all review comments at once for any given file, for several reasons:

  1. You usually want to address all the review comments at once while you are working on that file because they might guide you in a different direction when considered together than when addressing them one by one
  2. The comments trigger a discussion among reviewers & PR author and you want to see the whole context and the eventual resolution of that discussion (which might be different than the original review comment might have suggested ⚠️)

Therefore, I think that that seeing review comments from multiple reviewers for a given file is almost always preferred to seeing only specific people's comments. If the concern is "seeing too many comments at once" then I feel the ability to collapse review comments of other files/seeing only review comments for a single file at once would provide enough visual clarity & focus.

⚠️ I have never worked on a project where I would have to deal with more than 2 PR reviews (3 people collaborating), so my opinions might differ from those of who have worked on large scale projects like the Node.js repo etc.

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Feb 4, 2019

Could we implement per-review filtering in vy/prototype-sorted-by-file? Maybe have a checkbox or a toggling eye icon next to each review in the "Reviews" section, then only render review comment threads that include review comments from selected reviews. If you wanted to see the comments from a single review, you could filter it to only that one.

Here review summaries with checkboxes that lets you hide review comments from other reviews:

image

Hmmm.. 🤔 That would work, but feels a bit complex? Showing avatar and time for collapsed headers could already be enough to see which review comments "belong together"?

image

The last review comment ☝️ is not part of any review because it's a "single comment", but I think that doesn't really matter. I often make a review and then after submitting the review, make more single comments that should be "part" of the review. 🤷‍♂️

annthurium added some commits Mar 1, 2019

@smashwilson smashwilson referenced this pull request Mar 1, 2019

Draft

Reviews dock item #1995

### Files (tab)

Clicking on the "Files" tab displays the full, multi-file diff associated with the pull request. This is akin to the "Files changed" tab on dotcom.
### Files Changed (tab)

This comment has been minimized.

@annthurium

annthurium Mar 1, 2019

Contributor

I think we actually did call it "Files"

@@ -186,6 +201,14 @@ It was a great improvement, but filtering the diff with radio buttons and checkb
- Keep using an editable editor for the diffs, but add some padding.
- Introduce a "Reviews" footer to all sub-views to allow creating/submit a review, no matter where you are.

#### Third iteration

This comment has been minimized.

@annthurium

annthurium Mar 1, 2019

Contributor

this is actually our third iteration, right? I'm not like imagining things?

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Mar 1, 2019

Merging so the implementation PR #1995 has somewhere up-to-date to point 🎉

@smashwilson smashwilson merged commit 963c790 into master Mar 1, 2019

2 checks passed

codecov/patch Coverage not affected when comparing 91dac9a...51e2f84
Details
codecov/project 91.02% (-0.03%) compared to 91dac9a
Details

@smashwilson smashwilson deleted the aw/pr-review-update branch Mar 1, 2019

@vanessayuenn vanessayuenn referenced this pull request Mar 6, 2019

Closed

v0.27.0-0 QA Review #2005

7 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.