Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

PR Review Feature Request: review comment changes #1897

Merged
merged 20 commits into from Mar 1, 2019
Merged

PR Review Feature Request: review comment changes #1897

Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
3ff9662
Document using a "PullRequestReviewsItem" for review navigation.
smashwilson Jan 9, 2019
3bc411e
We actually called it "Files Changed," not "Files"
smashwilson Jan 9, 2019
11fb558
Describe the review summary panel within the "Files Changed" tab
smashwilson Jan 9, 2019
4696ec6
"Review summary list panel" concept
smashwilson Jan 9, 2019
3aeb59e
What I'm picturing for the PullRequestReviewsItem
smashwilson Jan 9, 2019
49d1212
Replace PR list
simurai Jan 17, 2019
6c6a5b6
Replace PR list actions
simurai Jan 17, 2019
b72a006
Replace header and footer
simurai Jan 17, 2019
5ca37fb
Update "Files" mockups
simurai Jan 17, 2019
dccb70a
Replace pull request reviews item
simurai Jan 17, 2019
7290720
Add inline diff mockup
simurai Jan 18, 2019
8db614d
add details of third iteration
Mar 1, 2019
37b09dc
remove section about review summary tab since we're not implementing …
Mar 1, 2019
145d276
"resolved conversation" buttons are now "mark as resolved" buttons.
Mar 1, 2019
bbb5f2c
the tab is current "files" not "files changed"
Mar 1, 2019
bf18eb7
add clarifying point about migrating PullRequestDetailView to dock
Mar 1, 2019
3c4580d
add new screenshot of PullRequestReviewsItem
Mar 1, 2019
4718550
add screenshot of expanded RCID comment
Mar 1, 2019
6dd3752
update footer screenshot
Mar 1, 2019
51e2f84
clarify button copy
Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
105 changes: 64 additions & 41 deletions docs/feature-requests/003-pull-request-review.md
View file
Open in desktop
Expand Up @@ -18,77 +18,53 @@ Peer review is also a critical part of the path to acceptance for pull requests

### Pull Request list

![image](https://user-images.githubusercontent.com/378023/46524357-89bf6580-c8c3-11e8-8e2d-ea02d5a1f278.png)
![image](https://user-images.githubusercontent.com/378023/51304737-4658c380-1a7c-11e9-8edb-7ceafeedabe5.png)

* Review progress is indicated for open pull requests listed in the GitHub panel.
* The pull request corresponding to the checked out branch gets special treatment in its own section at the top of the list.

![center pane](https://user-images.githubusercontent.com/378023/46985265-75c9fe00-d124-11e8-9b34-572cd1aaf701.png)
![center pane](https://user-images.githubusercontent.com/378023/51305096-45746180-1a7d-11e9-801b-37b3ab0c862a.png)

* Clicking a pull request in the list opens a `PullRequestDetailItem` in the workspace center.

* Clicking the progress bar opens a `PullRequestReviewsItem` in the left dock.

### PullRequestDetailItem

#### Header

![header](https://user-images.githubusercontent.com/378023/46536358-3829d180-c8e9-11e8-9167-3d1003ab566b.png)
![header](https://user-images.githubusercontent.com/378023/51305325-e400c280-1a7d-11e9-9b4e-b9cf2d326dd5.png)

At the top of each `PullRequestDetailItem` is a summary about the pull request, followed by the tabs to switch between different sub-views.

- Overview
- Files (**new**)
- Reviews (**new**)
- Commits
- Build Status

Below the tabs is a "tools bar" for controls to toggle review comments or collapse files.
Below the tabs is a "tools bar" with controls to toggle review comments or collapse files.

#### Footer

![reviews panel](https://user-images.githubusercontent.com/378023/46536010-17ad4780-c8e8-11e8-8338-338bb592efc5.png)
![reviews panel](https://user-images.githubusercontent.com/378023/51305353-f2e77500-1a7d-11e9-96f0-da879d49da3a.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:

Overview | Commits | Build Status
--- | --- | ---
![overview](https://user-images.githubusercontent.com/378023/46535907-ca30da80-c8e7-11e8-9401-2b8660d56c25.png) | ![commits](https://user-images.githubusercontent.com/378023/46535908-ca30da80-c8e7-11e8-87ca-01637f2554b6.png) | ![build status](https://user-images.githubusercontent.com/378023/46535909-cac97100-c8e7-11e8-8813-47fdaa3ece57.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.

When the pull request is checked out, an "open" button is shown in the review footer. Clicking "open" opens a `PullRequestReviewsItem` for this pull request's review comments as an item in the right workspace dock.

### 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](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png)
Clicking on the "Files Changed" tab displays the full, multi-file diff associated with the pull request. This is akin to the "Files changed" tab on dotcom.

* Diffs are editable.
* Editing the diff is _only_ possible if the pull request branch is checked out and the local branch history has not diverged from the remote branch history.

Uncollapsed (default) | Collapsed
--- | ---
![files](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png) | ![collapsed files](https://user-images.githubusercontent.com/378023/46931273-7069a680-d085-11e8-9ea7-c96a1772fe27.png)
![files](https://user-images.githubusercontent.com/378023/51305826-43ab9d80-1a7f-11e9-8b41-42bc4812d214.png)

* For large diffs, the files can be collapsed to get a better overview.

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.

### Reviews (tab)

Clicking on the "Reviews" tab shows all reviews of a pull request. This is akin to the review summaries that appear on the "Conversation" tab on dotcom. In addition, each review also includes review comments and their diff.

![reviews](https://user-images.githubusercontent.com/378023/46535563-c81a4c00-c8e6-11e8-9c0b-6ea575556101.png)
For large diffs, the files can be collapsed to get a better overview.

Uncollapsed (default) | Collapsed
--- | ---
![reviews](https://user-images.githubusercontent.com/378023/46535563-c81a4c00-c8e6-11e8-9c0b-6ea575556101.png) | ![collapsed reviews](https://user-images.githubusercontent.com/378023/46926357-62a72780-d06b-11e8-9344-23389d1c727c.png)

* Comments can be collapsed to get a better overview.
* Reviews get sorted by "urgency". Showing reviews that still need to get adressed at the top:
1. "recommended" changes
2. "commented" changes
3. "no review" (when a reviewer only leaves review comments, but no summary)
4. "approved" changes
5. "previous" reviews (when a reviewer made an earlier review and it's now out-dated)
* Within each group, sorting is done by "newest first".
![files](https://user-images.githubusercontent.com/378023/46536560-d3bb4200-c8e9-11e8-9764-dca0b84245cf.png) | ![collapsed files](https://user-images.githubusercontent.com/378023/46931273-7069a680-d085-11e8-9ea7-c96a1772fe27.png)

#### Create a new review

Expand Down Expand Up @@ -128,10 +104,49 @@ Clicking "Finish your review" from a comment or clicking "Review Changes" in the

![resolve a review](https://user-images.githubusercontent.com/378023/46927875-c08b3d80-d072-11e8-978b-024111312d79.png)

* Review comments can be resolved by clicking on the "Resolve conversation" buttons.
* Review comments can be resolved by clicking on the "Mark as resolved" buttons.
* If the "reply..." editor has non-whitespace content, it is submitted as a final comment first.

#### Context and navigation
### PullRequestReviewsItem

This item is opened in the workspace's right dock when the user:

* Clicks the review progress bar in the GitHub tab.
* Clicks the "open reviews" button on the review summary footer of a `PullRequestDetailItem`.
* Clicks the "<>" button on a review comment in the "Files Changed" tab of a `PullRequestDetailItem`.

It shows a scrollable view of all of the reviews and comments associated with a specific pull request,

![pull request reviews item](https://user-images.githubusercontent.com/3781742/53610984-c85f0080-3b81-11e9-9a82-9df43b6410f3.png)

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
3. "no review" (when a reviewer only leaves review comments, but no summary)
4. "approved" changes
5. "previous" reviews (when a reviewer made an earlier review and it's now out-dated)

Clicking on a review summary comment expands or collapses the associated review comments.

<img width="429" alt="screen shot 2019-02-28 at 6 03 50 pm" src="https://user-images.githubusercontent.com/3781742/53611421-5a1b3d80-3b83-11e9-9e50-ac4c54a67c13.png">

In addition to the comment, users see an abbreviated version of the diff, with 4 context lines.

Clicking on the "Jump To File" button opens a `TextEditor` on the corresponding position of the file under review. The clicked review comment is highlighted as the "current" one.

Clicking on the "View Changes" button opens the "Files" tab of the `PullRequestDetailsView`, so the user can see the full diff.


#### Within an open TextEditor

If an open `TextEditor` corresponds to a file that has one or more review comments in an open `PullRequestReviewsItem`, gutter and line decorations are added to the lines that match those review comment positions. The "current" one is styled differently to stand out.

![inline diff](https://user-images.githubusercontent.com/378023/51360052-68e6ed00-1b0d-11e9-852e-a51cff4d479e.png)

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

Review comments are shown in 3 different places. The comments themselves have the same functionality, but allow the comment to be seen in a different context, depending on different use cases. For example "reviewing a pull request", "addressing feedback", "editing the entire file".

Expand All @@ -145,8 +160,8 @@ In order to navigate between comments or switch context, each comment has the fo

* Clicking on the `<>` button in a review comment shows the comment in the entire file. If possible, the scroll-position is retained. This allows to quickly get more context about the code.
* If the current pull request is not checked out, the `<>` button is disabled, and a tooltip prompts the user to check out the pull request to edit the source.
* Clicking on the "sandwich" button shows the comment under the "Reviews" tab.
* Clicking on the "file-+" button (not shown in above screenshot) shows the comment under the "Files" tab.
* Clicking on the "sandwich" button shows the comment in the corresponding `PullRequestReviewsItem`.
* Clicking on the "file-+" button (not shown in above screenshot) shows the comment under the "Files Changed" tab.
* The up and down arrow buttons navigate to the next and previous unresolved review comments.
* Reaction emoji may be added to each comment with the "emoji" button. Existing emoji reaction tallies are included beneath each comment.

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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


Long comments can disrupt the code editing experience. Our third iteration keeps the review comments in a dock, a la Google Docs. This helps code authors more easily address comments, because they can see the comments and also get them out of the way.

Since this approach different from previous approaches, we performed a series of [usability studies](https://github.com/github/pe-editor-tools/blob/master/community/usability-testing/atom_rcid_research_summary.md) to validate that users would find this approach useful.

We may at some point want to migrate the entire PullRequestDetailView from the pane item to the dock, so as not to duplicate information. However, in the interest of getting code review in the editor shipped, we'll keep the pane item around in the short term.


## Unresolved questions

Expand Down