From 43b33e371421481d645d19ac59d7fc5c7539e758 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 26 Sep 2018 20:59:32 -0400 Subject: [PATCH 01/38] Screw it, press commit --- docs/rfcs/XXX-pull-request-review.md | 63 ++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 docs/rfcs/XXX-pull-request-review.md diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md new file mode 100644 index 0000000000..f536e84b27 --- /dev/null +++ b/docs/rfcs/XXX-pull-request-review.md @@ -0,0 +1,63 @@ +# Feature title + +Pull Request Review + +## Status + +Proposed + +## Summary + +Give or receive code reviews on pull requests within Atom. + +## Motivation + +Workflows around pull request reviews involve many trips between your editor and your browser. If you check out a pull request locally to test it and want to leave comments, you need to map the issues that you've found in your working copy back to lines on the diff to comment appropriately. Similarly, when you're given a review, you have to mentally correlate review comments on the diff on GitHub with the corresponding lines in your local working copy, then map _back_ to diff lines to respond once you've established context. By revealing review comments as decorations directly within the editor, we can eliminate all of these round-trips and streamline the review process for all involved. + +## Explanation + +### Entry points + +Reviews on the current pull request are rendered as a list on the current pull request tile. + +![review-list](https://user-images.githubusercontent.com/378023/44708426-1f582000-aae2-11e8-86bd-3074ae259e2d.png) + +* The review summary bubble is elided after the first sentence or N characters if necessary. +* Clicking the review summary bubble opens an `IssueishPaneItem` in the workspace center, open to the reviews tab. +* Clicking a line comment opens or activates an editor on the referenced file and scrolls to center the comment's line, translated according to local changes if appropriate. +* Line comments within the review are rendered: _with a dot_ before the file has been opened and the corresponding decoration is visible; _with no icon_ after the file and decoration have been seen; and _with a checkmark_ after the comment has been marked "resolved" with the control on its decoration. + +Pull request tiles other than the current pull request display a one-line review summary, showing the number of accepting, comment, and change-request reviews made on each. Clicking the review summary opens the `IssueishPaneItem` for that pull request and opens the review tab. + +> TODO: sketch here + +Each `IssueishPaneItem` opened on a pull request has a "Reviews" tab that shows the active reviews. + +## Drawbacks + + + +## Rationale and alternatives + + + +## Unresolved questions + + + +## Implementation phases + + From 6af2960ca81578f3b58b6c8491ffc86425583167 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 27 Sep 2018 10:03:22 -0400 Subject: [PATCH 02/38] and not or --- docs/rfcs/XXX-pull-request-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index f536e84b27..2955152a26 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -8,7 +8,7 @@ Proposed ## Summary -Give or receive code reviews on pull requests within Atom. +Give and receive code reviews on pull requests within Atom. ## Motivation From 48a31dcbd6507d6f5ec1f247cb2ed578a04e3437 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 28 Sep 2018 12:41:46 -0400 Subject: [PATCH 03/38] A little more justification --- docs/rfcs/XXX-pull-request-review.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 2955152a26..43cc07ab34 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -14,6 +14,8 @@ Give and receive code reviews on pull requests within Atom. Workflows around pull request reviews involve many trips between your editor and your browser. If you check out a pull request locally to test it and want to leave comments, you need to map the issues that you've found in your working copy back to lines on the diff to comment appropriately. Similarly, when you're given a review, you have to mentally correlate review comments on the diff on GitHub with the corresponding lines in your local working copy, then map _back_ to diff lines to respond once you've established context. By revealing review comments as decorations directly within the editor, we can eliminate all of these round-trips and streamline the review process for all involved. +Peer review is also a critical part of the path to acceptance for pull requests in many common workflows. By surfacing progress through code review, we provide context on the progress of each unit of work alongside existing indicators like commit status. + ## Explanation ### Entry points From 438a9e662d235df973754b9c11cf0fde7111df18 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 28 Sep 2018 12:43:59 -0400 Subject: [PATCH 04/38] Prose for the rest of the UI components --- docs/rfcs/XXX-pull-request-review.md | 42 ++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 43cc07ab34..92496ab956 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -18,7 +18,7 @@ Peer review is also a critical part of the path to acceptance for pull requests ## Explanation -### Entry points +### Current pull request tile Reviews on the current pull request are rendered as a list on the current pull request tile. @@ -29,11 +29,47 @@ Reviews on the current pull request are rendered as a list on the current pull r * Clicking a line comment opens or activates an editor on the referenced file and scrolls to center the comment's line, translated according to local changes if appropriate. * Line comments within the review are rendered: _with a dot_ before the file has been opened and the corresponding decoration is visible; _with no icon_ after the file and decoration have been seen; and _with a checkmark_ after the comment has been marked "resolved" with the control on its decoration. -Pull request tiles other than the current pull request display a one-line review summary, showing the number of accepting, comment, and change-request reviews made on each. Clicking the review summary opens the `IssueishPaneItem` for that pull request and opens the review tab. +### Non-current pull request tiles + +Pull request tiles other than the current pull request display a one-line review summary, showing the number of accepting, comment, and change-request reviews made on each. Clicking the review summary opens the `IssueishPaneItem` for that pull request and opens the reviews tab. + +> TODO: sketch here + +### IssueishPaneItem "Changes" tab + +Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows the full PR diff, annotated with comments from all reviews. + +![changes-tab](https://user-images.githubusercontent.com/378023/44789879-ba332600-abd8-11e8-9247-a19015ccd760.png) + +* The up and down arrow buttons quickly scroll to center the next or previous comment within this tab. +* Clicking the :hamburger: button navigates to the "Reviews" tab, expands the owning review, and scrolls to the same comment within that view. +* Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. +* Clicking within the "Reply..." text editor expands the editor to several lines and focuses it. +* Clicking "mark as resolved" marks the comment as resolved with on GitHub. If the "reply..." editor has non-whitespace content, it is submitted as a final comment first. +* The "comment" button is disabled unless the "reply" editor is expanded and has non-whitespace content. +* Clicking "comment" submits the response as a new stand-alone comment on that thread. + +### IssueishPaneItem "Reviews" tab + +Additionally, each has a "Reviews" tab that shows all reviews associated with this pull request in an accordion-style list. Unexpanded, each review is shown as its full summary comment and chosen outcome (comment, approve, or request changes). Expanded, its associated review comments are listed as well on their proximate diffs. > TODO: sketch here -Each `IssueishPaneItem` opened on a pull request has a "Reviews" tab that shows the active reviews. +* The "Mark as resolved" and "comment" buttons and the "reply" text areas match their behavior in the "Changes" tab. +* The up and down arrow buttons and :hamburger: button are not present here. +* The "code" (`<>`) button also behaves as it does on the "Changes" tab. + +### In-editor decorations + +When opening a TextEditor on a file that has been annotated with review comments on the current pull request, a block decoration is used to show the comment content at the corresponding position within the file content. Also, a gutter decoration is used to reveal lines that are included within the current pull requests' diff and may therefore include comments. + +![in-editor](https://user-images.githubusercontent.com/378023/44790482-69bcc800-abda-11e8-8a0f-922c0942b8c6.png) + +> TODO: add gutter decoration? + +* The comment's position is calculated from the position acquired by the GitHub API response, modified based on the git diff of that file (following renames) between the owning review's attached commit and the current state of the working copy (including any local modifications). Once created, the associated marker will also track unsaved modifications to the file in real time. +* The up and down arrow buttons navigate to the next and previous review comments within this review within their respective TextEditors. +* The "diff" button navigates to the "Reviews" tab of the corresponding pull request's `IssueishPaneItem`, expands the owning review, and scrolls to center the same comment within that view. ## Drawbacks From bcc742debe84b7f49c2f991db46980330365390b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 28 Sep 2018 13:31:20 -0400 Subject: [PATCH 05/38] Drawbacks and rationale --- docs/rfcs/XXX-pull-request-review.md | 50 ++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 92496ab956..88b50356b8 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -73,25 +73,49 @@ When opening a TextEditor on a file that has been annotated with review comments ## Drawbacks - +This adds a substantial amount of complexity to the UI, which is only justified for users that use GitHub pull request reviews. + +Showing all reviews in the current pull request tile can easily overwhelm the other pull request information included there. It also limits our ability to expand the information we provide there in the future (like associated issues, say). + +Rendering pull request comments within TextEditors can be intrusive: if there are many, or if your reviewers are particularly verbose, they could easily crowd out the code that you're trying to write and obscure your context. ## Rationale and alternatives - +One alternative may be to show review comments _only_ within the "changes" tab of an `IssueishPaneItem`. This simplifies flow considerably, because it removes the need to provide navigation among different views of the same review, and unifies the handling of current and non-current pull requests. However, I believe that the renderings of reviews in all three places each serve a unique purpose: + +* Reviews within the "Changes" tab of the `IssueishPaneItem` reveal a narrative formed by all of the reviews on a specific pull request together, as a conversation among reviewers. +* Reviews within the "Review" tab of the `IssueishPaneItem` reveal the narrative flow within each review individually. For example, review comments that refer to other comments within the same review (e.g. "same here" or "and again") become clearer here. +* Review comments within open TextEditors allow the reader to use more context within the source code to evaluate, address, or respond to each individual comment thread: consistency with functions that are not visible within the immediate diff, context within algorithms that span many lines. They also allow the receiver of a review to preserve the mental context of the review communication as they move back and forth between reading the content of a review and applying it to their source. ## Unresolved questions - +_Questions I expect to address before this is merged:_ + +How do we author new reviews within Atom? + +Can we access "draft" reviews from the GitHub API, to unify them between Atom and GitHub? + +How do we represent the resolution of a comment thread? Where can we reveal this progress through each review, and of all required reviews? + +Are there any design choices we can make to lessen the emotional weight of a "requests changes" review? Peer review has the most value when it discovers issues for the pull request author to address, but accepting criticism is a vulnerable moment. + +Similarly, are there any ways we can encourage empathy within the review authoring process? Can we encourage reviewers to make positive comments or demonstrate humility and open-mindedness? + +_Questions I expect to resolve throughout the implementation process:_ + +Review comment positioning within live TextEditors will be a tricky problem to address satisfactorily. What are the edge cases we need to handle there? + +The GraphQL API paths we need to interact with all involve multiple levels of pagination: pull requests, pull request reviews, review comments. How do we handle these within Relay? Or do we interact directly with GraphQL requests? + +How do we handle comment threads? + +_Questions I consider out of scope of this RFC:_ + +What other pull request information can we add to the GitHub pane item? + +Are there other tabs that we need within the `IssueishPaneItem`? + +How can we notify users when new information, including reviews, is available, preferably without being intrusive or disruptive? ## Implementation phases From e5410a31e8a9de99072c9efa8a9813d1af925180 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 28 Sep 2018 13:44:22 -0400 Subject: [PATCH 06/38] Review creation --- docs/rfcs/XXX-pull-request-review.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 88b50356b8..ba4cf9e5af 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -29,6 +29,14 @@ Reviews on the current pull request are rendered as a list on the current pull r * Clicking a line comment opens or activates an editor on the referenced file and scrolls to center the comment's line, translated according to local changes if appropriate. * Line comments within the review are rendered: _with a dot_ before the file has been opened and the corresponding decoration is visible; _with no icon_ after the file and decoration have been seen; and _with a checkmark_ after the comment has been marked "resolved" with the control on its decoration. +If a new review has been started locally, it appears at the top of the "Reviews" section within this tile: + +![pending-review](https://user-images.githubusercontent.com/378023/40404269-db852df6-5e91-11e8-9e16-bae433d3a5f9.png) + +* The review summary is a TextEditor that may be used to compose a summary comment. +* Choosing "Cancel" dismisses the review and any comments made. If there are local review comments that will be lost, a confirmation prompt is shown first. +* Choosing "Submit review" submits the drafted review to GitHub. + ### Non-current pull request tiles Pull request tiles other than the current pull request display a one-line review summary, showing the number of accepting, comment, and change-request reviews made on each. Clicking the review summary opens the `IssueishPaneItem` for that pull request and opens the reviews tab. @@ -49,6 +57,8 @@ Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows * The "comment" button is disabled unless the "reply" editor is expanded and has non-whitespace content. * Clicking "comment" submits the response as a new stand-alone comment on that thread. +Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new review with the same UI as described in the "In-editor decorations" section. + ### IssueishPaneItem "Reviews" tab Additionally, each has a "Reviews" tab that shows all reviews associated with this pull request in an accordion-style list. Unexpanded, each review is shown as its full summary comment and chosen outcome (comment, approve, or request changes). Expanded, its associated review comments are listed as well on their proximate diffs. @@ -59,6 +69,10 @@ Additionally, each has a "Reviews" tab that shows all reviews associated with th * The up and down arrow buttons and :hamburger: button are not present here. * The "code" (`<>`) button also behaves as it does on the "Changes" tab. +A local, pending review created by the user is also shown at the top of this list, with controls to edit its summary comment and choose its final state. + +> TODO: sketch here + ### In-editor decorations When opening a TextEditor on a file that has been annotated with review comments on the current pull request, a block decoration is used to show the comment content at the corresponding position within the file content. Also, a gutter decoration is used to reveal lines that are included within the current pull requests' diff and may therefore include comments. @@ -71,6 +85,14 @@ When opening a TextEditor on a file that has been annotated with review comments * The up and down arrow buttons navigate to the next and previous review comments within this review within their respective TextEditors. * The "diff" button navigates to the "Reviews" tab of the corresponding pull request's `IssueishPaneItem`, expands the owning review, and scrolls to center the same comment within that view. +Hovering along the gutter within a pull request diff region reveals a `+` icon, which may be clicked to begin a new review: + +![plus-icon](https://user-images.githubusercontent.com/378023/40348708-6698b2ea-5ddf-11e8-8eaa-9d95bc483fb1.png) + +Clicking the `+` reveals a new comment box, which may be used to submit a single comment or begin a multi-comment review: + +![single-review](https://user-images.githubusercontent.com/378023/40351475-78a527c2-5de7-11e8-8006-72d859514ecc.png) + ## Drawbacks This adds a substantial amount of complexity to the UI, which is only justified for users that use GitHub pull request reviews. From 3b7ac4a873458cc56fcd775db567acaab6a2e9de Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 28 Sep 2018 13:52:00 -0400 Subject: [PATCH 07/38] Use real headers --- docs/rfcs/XXX-pull-request-review.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index ba4cf9e5af..7884f96938 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -111,7 +111,7 @@ One alternative may be to show review comments _only_ within the "changes" tab o ## Unresolved questions -_Questions I expect to address before this is merged:_ +### Questions I expect to address before this is merged How do we author new reviews within Atom? @@ -123,7 +123,7 @@ Are there any design choices we can make to lessen the emotional weight of a "re Similarly, are there any ways we can encourage empathy within the review authoring process? Can we encourage reviewers to make positive comments or demonstrate humility and open-mindedness? -_Questions I expect to resolve throughout the implementation process:_ +### Questions I expect to resolve throughout the implementation process Review comment positioning within live TextEditors will be a tricky problem to address satisfactorily. What are the edge cases we need to handle there? @@ -131,7 +131,7 @@ The GraphQL API paths we need to interact with all involve multiple levels of pa How do we handle comment threads? -_Questions I consider out of scope of this RFC:_ +### Questions I consider out of scope of this RFC What other pull request information can we add to the GitHub pane item? From 85ba758e03c49ac7b5a8802439a948451917b2d0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 28 Sep 2018 13:52:38 -0400 Subject: [PATCH 08/38] Kind of answered that one myself --- docs/rfcs/XXX-pull-request-review.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 7884f96938..1bfb65741e 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -113,8 +113,6 @@ One alternative may be to show review comments _only_ within the "changes" tab o ### Questions I expect to address before this is merged -How do we author new reviews within Atom? - Can we access "draft" reviews from the GitHub API, to unify them between Atom and GitHub? How do we represent the resolution of a comment thread? Where can we reveal this progress through each review, and of all required reviews? From 194d36295371071b467a0f49ab08db14c3d2697b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 28 Sep 2018 15:11:26 -0400 Subject: [PATCH 09/38] Add a mock-up for non-current pull request tiles --- docs/rfcs/XXX-pull-request-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 1bfb65741e..26178edf59 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -41,7 +41,7 @@ If a new review has been started locally, it appears at the top of the "Reviews" Pull request tiles other than the current pull request display a one-line review summary, showing the number of accepting, comment, and change-request reviews made on each. Clicking the review summary opens the `IssueishPaneItem` for that pull request and opens the reviews tab. -> TODO: sketch here +![non-current pull request tile](https://user-images.githubusercontent.com/17565/46228625-a2aea080-c330-11e8-945b-72be7824623f.png) ### IssueishPaneItem "Changes" tab From 81a53e07dc5375edd3af12e9b1974d6257489863 Mon Sep 17 00:00:00 2001 From: simurai Date: Mon, 1 Oct 2018 15:44:10 +0900 Subject: [PATCH 10/38] Update review-list mockup --- docs/rfcs/XXX-pull-request-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 26178edf59..aa2cc0ab82 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -22,7 +22,7 @@ Peer review is also a critical part of the path to acceptance for pull requests Reviews on the current pull request are rendered as a list on the current pull request tile. -![review-list](https://user-images.githubusercontent.com/378023/44708426-1f582000-aae2-11e8-86bd-3074ae259e2d.png) +![review-list](https://user-images.githubusercontent.com/378023/46273505-b9533280-c590-11e8-840e-a8eac8023cad.png) * The review summary bubble is elided after the first sentence or N characters if necessary. * Clicking the review summary bubble opens an `IssueishPaneItem` in the workspace center, open to the reviews tab. From 12532be1ff3a9489d3fd842a9b6be44ffe544f7a Mon Sep 17 00:00:00 2001 From: simurai Date: Mon, 1 Oct 2018 16:22:58 +0900 Subject: [PATCH 11/38] Add reviews-tab mockup --- docs/rfcs/XXX-pull-request-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index aa2cc0ab82..51b7f2283c 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -63,7 +63,7 @@ Hovering in the diff's gutter reveals a `+` icon that allows users to begin crea Additionally, each has a "Reviews" tab that shows all reviews associated with this pull request in an accordion-style list. Unexpanded, each review is shown as its full summary comment and chosen outcome (comment, approve, or request changes). Expanded, its associated review comments are listed as well on their proximate diffs. -> TODO: sketch here +![reviews-tab](https://user-images.githubusercontent.com/378023/46274944-20bfb100-c596-11e8-83c0-363904ca7d5f.png) * The "Mark as resolved" and "comment" buttons and the "reply" text areas match their behavior in the "Changes" tab. * The up and down arrow buttons and :hamburger: button are not present here. From 3279a2692d363a157b7d286f405a1f312ea0d765 Mon Sep 17 00:00:00 2001 From: simurai Date: Mon, 1 Oct 2018 16:48:01 +0900 Subject: [PATCH 12/38] Update pending-review mockup --- docs/rfcs/XXX-pull-request-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 51b7f2283c..70dd5005e5 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -31,7 +31,7 @@ Reviews on the current pull request are rendered as a list on the current pull r If a new review has been started locally, it appears at the top of the "Reviews" section within this tile: -![pending-review](https://user-images.githubusercontent.com/378023/40404269-db852df6-5e91-11e8-9e16-bae433d3a5f9.png) +![pending-review](https://user-images.githubusercontent.com/378023/46275946-9bd69680-c599-11e8-9889-66c35458286a.png) * The review summary is a TextEditor that may be used to compose a summary comment. * Choosing "Cancel" dismisses the review and any comments made. If there are local review comments that will be lost, a confirmation prompt is shown first. From d902dcfee9f6e76775827d5e659696fc2c48d8b0 Mon Sep 17 00:00:00 2001 From: simurai Date: Mon, 1 Oct 2018 21:06:57 +0900 Subject: [PATCH 13/38] Replace changes and reviews tab mockups --- docs/rfcs/XXX-pull-request-review.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 70dd5005e5..8489d9f2eb 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -47,7 +47,7 @@ Pull request tiles other than the current pull request display a one-line review Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows the full PR diff, annotated with comments from all reviews. -![changes-tab](https://user-images.githubusercontent.com/378023/44789879-ba332600-abd8-11e8-9247-a19015ccd760.png) +![changes-tab](https://user-images.githubusercontent.com/378023/46287431-6e9bdf80-c5bd-11e8-99eb-f3f81ba64e81.png) * The up and down arrow buttons quickly scroll to center the next or previous comment within this tab. * Clicking the :hamburger: button navigates to the "Reviews" tab, expands the owning review, and scrolls to the same comment within that view. @@ -63,7 +63,7 @@ Hovering in the diff's gutter reveals a `+` icon that allows users to begin crea Additionally, each has a "Reviews" tab that shows all reviews associated with this pull request in an accordion-style list. Unexpanded, each review is shown as its full summary comment and chosen outcome (comment, approve, or request changes). Expanded, its associated review comments are listed as well on their proximate diffs. -![reviews-tab](https://user-images.githubusercontent.com/378023/46274944-20bfb100-c596-11e8-83c0-363904ca7d5f.png) +![reviews-tab](https://user-images.githubusercontent.com/378023/46287432-6e9bdf80-c5bd-11e8-8290-50dd17a2c733.png) * The "Mark as resolved" and "comment" buttons and the "reply" text areas match their behavior in the "Changes" tab. * The up and down arrow buttons and :hamburger: button are not present here. From 64f2433654ae107ca0632be8649eb33f94721a10 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 11:12:00 -0400 Subject: [PATCH 14/38] "position: sticky" navigation and filter banner in "Changes" tab Co-Authored-By: Katrina Uychaco --- docs/rfcs/XXX-pull-request-review.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 8489d9f2eb..3cb281e65d 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -45,10 +45,11 @@ Pull request tiles other than the current pull request display a one-line review ### IssueishPaneItem "Changes" tab -Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows the full PR diff, annotated with comments from all reviews. +Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows the full PR diff, annotated with comments from all reviews. A banner at the top of the diff offers navigation to individual files within the diff and to individual review comments, allows each review to be hidden or shown with a filter control, and shows a progress bar that counts "resolved" review comments. ![changes-tab](https://user-images.githubusercontent.com/378023/46287431-6e9bdf80-c5bd-11e8-99eb-f3f81ba64e81.png) +* The navigation and filter banner remains visible as the "Changes" tab is scrolled. * The up and down arrow buttons quickly scroll to center the next or previous comment within this tab. * Clicking the :hamburger: button navigates to the "Reviews" tab, expands the owning review, and scrolls to the same comment within that view. * Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. From 0cb878eb15f30e033b7242e0a023b8ed21ca9551 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 11:15:55 -0400 Subject: [PATCH 15/38] Unify the "Reviews" and "Changes" tabs Co-Authored-By: Katrina Uychaco Co-Authored-By: Vanessa Yuen Co-Authored-By: simurai --- docs/rfcs/XXX-pull-request-review.md | 30 +++++----------------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 3cb281e65d..1ad9e6266e 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -39,7 +39,7 @@ If a new review has been started locally, it appears at the top of the "Reviews" ### Non-current pull request tiles -Pull request tiles other than the current pull request display a one-line review summary, showing the number of accepting, comment, and change-request reviews made on each. Clicking the review summary opens the `IssueishPaneItem` for that pull request and opens the reviews tab. +Pull request tiles other than the current pull request display a one-line review summary, showing the number of accepting, comment, and change-request reviews made on each. Clicking the review summary opens the `IssueishPaneItem` for that pull request and opens the "Changes" tab. ![non-current pull request tile](https://user-images.githubusercontent.com/17565/46228625-a2aea080-c330-11e8-945b-72be7824623f.png) @@ -51,28 +51,12 @@ Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows * The navigation and filter banner remains visible as the "Changes" tab is scrolled. * The up and down arrow buttons quickly scroll to center the next or previous comment within this tab. -* Clicking the :hamburger: button navigates to the "Reviews" tab, expands the owning review, and scrolls to the same comment within that view. -* Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. -* Clicking within the "Reply..." text editor expands the editor to several lines and focuses it. +* Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. If the current pull request is not checked out, the "code" button is disabled, and a tooltip prompts the user to check out the pull request to edit the source. * Clicking "mark as resolved" marks the comment as resolved with on GitHub. If the "reply..." editor has non-whitespace content, it is submitted as a final comment first. * The "comment" button is disabled unless the "reply" editor is expanded and has non-whitespace content. * Clicking "comment" submits the response as a new stand-alone comment on that thread. -Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new review with the same UI as described in the "In-editor decorations" section. - -### IssueishPaneItem "Reviews" tab - -Additionally, each has a "Reviews" tab that shows all reviews associated with this pull request in an accordion-style list. Unexpanded, each review is shown as its full summary comment and chosen outcome (comment, approve, or request changes). Expanded, its associated review comments are listed as well on their proximate diffs. - -![reviews-tab](https://user-images.githubusercontent.com/378023/46287432-6e9bdf80-c5bd-11e8-8290-50dd17a2c733.png) - -* The "Mark as resolved" and "comment" buttons and the "reply" text areas match their behavior in the "Changes" tab. -* The up and down arrow buttons and :hamburger: button are not present here. -* The "code" (`<>`) button also behaves as it does on the "Changes" tab. - -A local, pending review created by the user is also shown at the top of this list, with controls to edit its summary comment and choose its final state. - -> TODO: sketch here +Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new review with the same UI as described in ["In-editor decorations"](#in-editor-decorations). ### In-editor decorations @@ -84,7 +68,7 @@ When opening a TextEditor on a file that has been annotated with review comments * The comment's position is calculated from the position acquired by the GitHub API response, modified based on the git diff of that file (following renames) between the owning review's attached commit and the current state of the working copy (including any local modifications). Once created, the associated marker will also track unsaved modifications to the file in real time. * The up and down arrow buttons navigate to the next and previous review comments within this review within their respective TextEditors. -* The "diff" button navigates to the "Reviews" tab of the corresponding pull request's `IssueishPaneItem`, expands the owning review, and scrolls to center the same comment within that view. +* The "diff" button navigates to the "Changes" tab of the corresponding pull request's `IssueishPaneItem`, expands the owning review, and scrolls to center the same comment within that view. Hovering along the gutter within a pull request diff region reveals a `+` icon, which may be clicked to begin a new review: @@ -104,11 +88,7 @@ Rendering pull request comments within TextEditors can be intrusive: if there ar ## Rationale and alternatives -One alternative may be to show review comments _only_ within the "changes" tab of an `IssueishPaneItem`. This simplifies flow considerably, because it removes the need to provide navigation among different views of the same review, and unifies the handling of current and non-current pull requests. However, I believe that the renderings of reviews in all three places each serve a unique purpose: - -* Reviews within the "Changes" tab of the `IssueishPaneItem` reveal a narrative formed by all of the reviews on a specific pull request together, as a conversation among reviewers. -* Reviews within the "Review" tab of the `IssueishPaneItem` reveal the narrative flow within each review individually. For example, review comments that refer to other comments within the same review (e.g. "same here" or "and again") become clearer here. -* Review comments within open TextEditors allow the reader to use more context within the source code to evaluate, address, or respond to each individual comment thread: consistency with functions that are not visible within the immediate diff, context within algorithms that span many lines. They also allow the receiver of a review to preserve the mental context of the review communication as they move back and forth between reading the content of a review and applying it to their source. + ## Unresolved questions From e50fa722dddc251f10d0bc74e8f070033f95143a Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 11:19:07 -0400 Subject: [PATCH 16/38] Emphasize review authoring and finalization on the "Changes" tab Co-Authored-By: Katrina Uychaco Co-Authored-By: simurai --- docs/rfcs/XXX-pull-request-review.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 1ad9e6266e..f9afde3086 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -58,6 +58,8 @@ Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new review with the same UI as described in ["In-editor decorations"](#in-editor-decorations). +If a pending review is present, its comments are also shown and editable here. A pending review may be finalized by submitting a form that appears at the end of the existing review summary comment list. + ### In-editor decorations When opening a TextEditor on a file that has been annotated with review comments on the current pull request, a block decoration is used to show the comment content at the corresponding position within the file content. Also, a gutter decoration is used to reveal lines that are included within the current pull requests' diff and may therefore include comments. From 884c39db337a1d0ae33ed1796d312102fac0a3c1 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 11:20:00 -0400 Subject: [PATCH 17/38] Mention the review summary list --- docs/rfcs/XXX-pull-request-review.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index f9afde3086..495d91a407 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -45,7 +45,9 @@ Pull request tiles other than the current pull request display a one-line review ### IssueishPaneItem "Changes" tab -Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows the full PR diff, annotated with comments from all reviews. A banner at the top of the diff offers navigation to individual files within the diff and to individual review comments, allows each review to be hidden or shown with a filter control, and shows a progress bar that counts "resolved" review comments. +Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows the full PR diff, annotated with comments from all reviews. + +Summary comments for each existing review appear in a list before the PR diff's body. A banner at the top of the diff offers navigation to individual files within the diff and to individual review comments, allows each review to be hidden or shown with a filter control, and shows a progress bar that counts "resolved" review comments. ![changes-tab](https://user-images.githubusercontent.com/378023/46287431-6e9bdf80-c5bd-11e8-99eb-f3f81ba64e81.png) From c37fe1fe990d13b13f714e53926fd04aae90e5f3 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 11:21:23 -0400 Subject: [PATCH 18/38] Reaction emoji ftw :cake: :tada: Co-Authored-By: Tilde Ann Thurium --- docs/rfcs/XXX-pull-request-review.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 495d91a407..239ab8c7fe 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -54,6 +54,7 @@ Summary comments for each existing review appear in a list before the PR diff's * The navigation and filter banner remains visible as the "Changes" tab is scrolled. * The up and down arrow buttons quickly scroll to center the next or previous comment within this tab. * Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. If the current pull request is not checked out, the "code" button is disabled, and a tooltip prompts the user to check out the pull request to edit the source. +* Reaction emoji may be added to each comment with the "emoji" button. Existing emoji reaction tallies are included beneath each comment. * Clicking "mark as resolved" marks the comment as resolved with on GitHub. If the "reply..." editor has non-whitespace content, it is submitted as a final comment first. * The "comment" button is disabled unless the "reply" editor is expanded and has non-whitespace content. * Clicking "comment" submits the response as a new stand-alone comment on that thread. From c45f9deed2246d6849003782d6a63ae7a8137c29 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 11:24:58 -0400 Subject: [PATCH 19/38] Explicit "single one-off comment" handling Co-Authored-By: Tilde Ann Thurium --- docs/rfcs/XXX-pull-request-review.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 239ab8c7fe..979f1b47b1 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -59,9 +59,7 @@ Summary comments for each existing review appear in a list before the PR diff's * The "comment" button is disabled unless the "reply" editor is expanded and has non-whitespace content. * Clicking "comment" submits the response as a new stand-alone comment on that thread. -Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new review with the same UI as described in ["In-editor decorations"](#in-editor-decorations). - -If a pending review is present, its comments are also shown and editable here. A pending review may be finalized by submitting a form that appears at the end of the existing review summary comment list. +Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new review, or making an isolated comment, using the same UI described in ["In-editor decorations"](#in-editor-decorations). If a pending review is present, its comments are also shown and editable here. A pending review may be finalized by submitting a form that appears at the end of the existing review summary comment list. ### In-editor decorations @@ -83,6 +81,10 @@ Clicking the `+` reveals a new comment box, which may be used to submit a single ![single-review](https://user-images.githubusercontent.com/378023/40351475-78a527c2-5de7-11e8-8006-72d859514ecc.png) +* If a draft review is already in progress, the "Add single comment" button is disabled and the "Start a review" button reads "Add review comment". +* Clicking "Add single comment" submits a non-review diff comment and does not create a draft review. +* Clicking "Start a review" creates a draft review and attaches the authored comment to it. + ## Drawbacks This adds a substantial amount of complexity to the UI, which is only justified for users that use GitHub pull request reviews. From 792d9d0875424a19d3b24aa0c01434123829536e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 11:35:21 -0400 Subject: [PATCH 20/38] Add answers to some of our questions :smile: --- docs/rfcs/XXX-pull-request-review.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 979f1b47b1..4b1f7e348d 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -105,14 +105,24 @@ Can we access "draft" reviews from the GitHub API, to unify them between Atom an How do we represent the resolution of a comment thread? Where can we reveal this progress through each review, and of all required reviews? +* _We'll show a progress bar on a sticky header at the top of the "Changes" tab within each `IssueishPaneItem`._ + Are there any design choices we can make to lessen the emotional weight of a "requests changes" review? Peer review has the most value when it discovers issues for the pull request author to address, but accepting criticism is a vulnerable moment. +* _Chosing phrasing and iconography carefully for "recommend changes"._ + Similarly, are there any ways we can encourage empathy within the review authoring process? Can we encourage reviewers to make positive comments or demonstrate humility and open-mindedness? +* _Emoji reactions on comments :cake: :tada:_ +* _Enable integration with Teletype for smoother jumping to a synchronous review_ + ### Questions I expect to resolve throughout the implementation process Review comment positioning within live TextEditors will be a tricky problem to address satisfactorily. What are the edge cases we need to handle there? +* _Review comments on deleted lines._ +* _Review comments on deleted files._ + The GraphQL API paths we need to interact with all involve multiple levels of pagination: pull requests, pull request reviews, review comments. How do we handle these within Relay? Or do we interact directly with GraphQL requests? How do we handle comment threads? From 5465f715d11b6800691455abe75183f89a1c8c38 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 11:45:29 -0400 Subject: [PATCH 21/38] Oh hey that works --- docs/rfcs/XXX-pull-request-review.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 4b1f7e348d..542cc20490 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -103,6 +103,8 @@ Rendering pull request comments within TextEditors can be intrusive: if there ar Can we access "draft" reviews from the GitHub API, to unify them between Atom and GitHub? +* _Yes, the `reviews` object includes it in a `PENDING` state._ + How do we represent the resolution of a comment thread? Where can we reveal this progress through each review, and of all required reviews? * _We'll show a progress bar on a sticky header at the top of the "Changes" tab within each `IssueishPaneItem`._ From ea33834c652dffa73358f2d85901d3c8668642e2 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 12:01:17 -0400 Subject: [PATCH 22/38] Dependency graph why not --- docs/rfcs/XXX-pull-request-review.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 542cc20490..f267581e97 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -139,7 +139,4 @@ How can we notify users when new information, including reviews, is available, p ## Implementation phases - +![dependency-graph](https://user-images.githubusercontent.com/17565/46361100-d47a7c80-c63a-11e8-83de-4a548be9cb9c.png) From d4b153bbee52254e6e01582a25e95e3abf1942a8 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 14:33:37 -0400 Subject: [PATCH 23/38] Update iconography description --- docs/rfcs/XXX-pull-request-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index f267581e97..f403323571 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -27,7 +27,7 @@ Reviews on the current pull request are rendered as a list on the current pull r * The review summary bubble is elided after the first sentence or N characters if necessary. * Clicking the review summary bubble opens an `IssueishPaneItem` in the workspace center, open to the reviews tab. * Clicking a line comment opens or activates an editor on the referenced file and scrolls to center the comment's line, translated according to local changes if appropriate. -* Line comments within the review are rendered: _with a dot_ before the file has been opened and the corresponding decoration is visible; _with no icon_ after the file and decoration have been seen; and _with a checkmark_ after the comment has been marked "resolved" with the control on its decoration. +* Line comments within the review are rendered: _with a vertical blue bar_ before the file has been opened and the corresponding decoration is visible; _with a vertical grey bar_ after the file and decoration have been seen; and _with a vertical green bar_ after the comment has been marked "resolved" with the control on its decoration. If a new review has been started locally, it appears at the top of the "Reviews" section within this tile: From 6bccb4b48a6420353440b13e60b95fdd3935656d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 14:37:49 -0400 Subject: [PATCH 24/38] Suggestion for reducing confusion when a different PR is open Co-Authored-By: Katrina Uychaco --- docs/rfcs/XXX-pull-request-review.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index f403323571..b185916ba7 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -28,6 +28,7 @@ Reviews on the current pull request are rendered as a list on the current pull r * Clicking the review summary bubble opens an `IssueishPaneItem` in the workspace center, open to the reviews tab. * Clicking a line comment opens or activates an editor on the referenced file and scrolls to center the comment's line, translated according to local changes if appropriate. * Line comments within the review are rendered: _with a vertical blue bar_ before the file has been opened and the corresponding decoration is visible; _with a vertical grey bar_ after the file and decoration have been seen; and _with a vertical green bar_ after the comment has been marked "resolved" with the control on its decoration. +* The review summary bubble and line comment lists are greyed out if a different `IssueishPaneItem` is activated. If a new review has been started locally, it appears at the top of the "Reviews" section within this tile: From 759884aab3eb5993c035618f24f4498516391bf5 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 14:41:11 -0400 Subject: [PATCH 25/38] Remove non-current pull request tile changes --- docs/rfcs/XXX-pull-request-review.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index b185916ba7..48ab3e97ed 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -38,12 +38,6 @@ If a new review has been started locally, it appears at the top of the "Reviews" * Choosing "Cancel" dismisses the review and any comments made. If there are local review comments that will be lost, a confirmation prompt is shown first. * Choosing "Submit review" submits the drafted review to GitHub. -### Non-current pull request tiles - -Pull request tiles other than the current pull request display a one-line review summary, showing the number of accepting, comment, and change-request reviews made on each. Clicking the review summary opens the `IssueishPaneItem` for that pull request and opens the "Changes" tab. - -![non-current pull request tile](https://user-images.githubusercontent.com/17565/46228625-a2aea080-c330-11e8-945b-72be7824623f.png) - ### IssueishPaneItem "Changes" tab Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows the full PR diff, annotated with comments from all reviews. From af4c794b24d42ac0df970e40f1702fa60136802b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 15:15:17 -0400 Subject: [PATCH 26/38] IssueishDetailItem not IssueishPaneItem --- docs/rfcs/XXX-pull-request-review.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 48ab3e97ed..8f02f8c945 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -25,10 +25,10 @@ Reviews on the current pull request are rendered as a list on the current pull r ![review-list](https://user-images.githubusercontent.com/378023/46273505-b9533280-c590-11e8-840e-a8eac8023cad.png) * The review summary bubble is elided after the first sentence or N characters if necessary. -* Clicking the review summary bubble opens an `IssueishPaneItem` in the workspace center, open to the reviews tab. +* Clicking the review summary bubble opens an `IssueishDetailItem` in the workspace center. * Clicking a line comment opens or activates an editor on the referenced file and scrolls to center the comment's line, translated according to local changes if appropriate. * Line comments within the review are rendered: _with a vertical blue bar_ before the file has been opened and the corresponding decoration is visible; _with a vertical grey bar_ after the file and decoration have been seen; and _with a vertical green bar_ after the comment has been marked "resolved" with the control on its decoration. -* The review summary bubble and line comment lists are greyed out if a different `IssueishPaneItem` is activated. +* The review summary bubble and line comment lists are greyed out if a different `IssueishDetailItem` is activated. If a new review has been started locally, it appears at the top of the "Reviews" section within this tile: @@ -66,7 +66,7 @@ When opening a TextEditor on a file that has been annotated with review comments * The comment's position is calculated from the position acquired by the GitHub API response, modified based on the git diff of that file (following renames) between the owning review's attached commit and the current state of the working copy (including any local modifications). Once created, the associated marker will also track unsaved modifications to the file in real time. * The up and down arrow buttons navigate to the next and previous review comments within this review within their respective TextEditors. -* The "diff" button navigates to the "Changes" tab of the corresponding pull request's `IssueishPaneItem`, expands the owning review, and scrolls to center the same comment within that view. +* The "diff" button navigates to the corresponding pull request's `IssueishDetailItem` and scrolls to center the same comment within that view. Hovering along the gutter within a pull request diff region reveals a `+` icon, which may be clicked to begin a new review: @@ -102,7 +102,7 @@ Can we access "draft" reviews from the GitHub API, to unify them between Atom an How do we represent the resolution of a comment thread? Where can we reveal this progress through each review, and of all required reviews? -* _We'll show a progress bar on a sticky header at the top of the "Changes" tab within each `IssueishPaneItem`._ +* _We'll show a progress bar on a sticky header at the top of the `IssueishDetailItem`._ Are there any design choices we can make to lessen the emotional weight of a "requests changes" review? Peer review has the most value when it discovers issues for the pull request author to address, but accepting criticism is a vulnerable moment. @@ -128,7 +128,7 @@ How do we handle comment threads? What other pull request information can we add to the GitHub pane item? -Are there other tabs that we need within the `IssueishPaneItem`? +Are there other tabs that we need within the `IssueishDetailItem`? How can we notify users when new information, including reviews, is available, preferably without being intrusive or disruptive? From 6313744bc005e9c799de72eeccf253ec0c2f244e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 15:22:02 -0400 Subject: [PATCH 27/38] Include @kuychaco's IssueishDetailItem refocus idea :lightbulb: I also rolled in @sguthal's suggestion to allow you to create a new review here while I was at it. Co-Authored-By: Katrina Uychaco Co-Authored-By: Sarah Guthals --- docs/rfcs/XXX-pull-request-review.md | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 8f02f8c945..7b02df0b89 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -38,15 +38,27 @@ If a new review has been started locally, it appears at the top of the "Reviews" * Choosing "Cancel" dismisses the review and any comments made. If there are local review comments that will be lost, a confirmation prompt is shown first. * Choosing "Submit review" submits the drafted review to GitHub. -### IssueishPaneItem "Changes" tab +### IssueishDetailItem -Each `IssueishPaneItem` opened on a pull request has a "Changes" tab that shows the full PR diff, annotated with comments from all reviews. +Each `IssueishDetailItem` opened on a pull request displays the full, multi-file diff associated with the pull request. -Summary comments for each existing review appear in a list before the PR diff's body. A banner at the top of the diff offers navigation to individual files within the diff and to individual review comments, allows each review to be hidden or shown with a filter control, and shows a progress bar that counts "resolved" review comments. +A banner at the top of the pane offers navigation to individual files within the diff and to individual review comments, allows each review to be hidden or shown with a filter control, and shows a progress bar that counts "resolved" review comments. The banner remains visible as you scroll the pane. + +At the top of the pane is the existing summary box: + +issueish-detail-item pane + +* Clicking on the build status summary icon (green checkmark, donut chart, or X) expands an ephemeral panel beneath the summary box showing build review status. Clicking the icon again or clicking on "dismiss" dismisses it. +* Clicking on the commit count opens the log view to those commits. + +Summary comments for each existing review appear in a list below that. If a pending review is being drafted, it appears at the end of the list; otherwise, a control is present to create one. A pending review may be finalized by submitting a form that appears adjacent to it. + +After the summary comments, the diff is shown, with review comments in place: ![changes-tab](https://user-images.githubusercontent.com/378023/46287431-6e9bdf80-c5bd-11e8-99eb-f3f81ba64e81.png) -* The navigation and filter banner remains visible as the "Changes" tab is scrolled. +On each review comment decoration: + * The up and down arrow buttons quickly scroll to center the next or previous comment within this tab. * Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. If the current pull request is not checked out, the "code" button is disabled, and a tooltip prompts the user to check out the pull request to edit the source. * Reaction emoji may be added to each comment with the "emoji" button. Existing emoji reaction tallies are included beneath each comment. @@ -54,7 +66,7 @@ Summary comments for each existing review appear in a list before the PR diff's * The "comment" button is disabled unless the "reply" editor is expanded and has non-whitespace content. * Clicking "comment" submits the response as a new stand-alone comment on that thread. -Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new review, or making an isolated comment, using the same UI described in ["In-editor decorations"](#in-editor-decorations). If a pending review is present, its comments are also shown and editable here. A pending review may be finalized by submitting a form that appears at the end of the existing review summary comment list. +Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new pending review, or making an isolated comment, using the same UI described in ["In-editor decorations"](#in-editor-decorations). If a pending review is present, its comments are also shown and editable here. ### In-editor decorations From 2ab74b59873c3b5bccac7ef679795eb483b335cf Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 2 Oct 2018 15:34:09 -0400 Subject: [PATCH 28/38] Create a pending review if one is not present Co-Authored-By: Sarah Guthals --- docs/rfcs/XXX-pull-request-review.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 7b02df0b89..95003ecec4 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -30,7 +30,7 @@ Reviews on the current pull request are rendered as a list on the current pull r * Line comments within the review are rendered: _with a vertical blue bar_ before the file has been opened and the corresponding decoration is visible; _with a vertical grey bar_ after the file and decoration have been seen; and _with a vertical green bar_ after the comment has been marked "resolved" with the control on its decoration. * The review summary bubble and line comment lists are greyed out if a different `IssueishDetailItem` is activated. -If a new review has been started locally, it appears at the top of the "Reviews" section within this tile: +If a pending review is present, it appears at the top of the "Reviews" section within this tile: ![pending-review](https://user-images.githubusercontent.com/378023/46275946-9bd69680-c599-11e8-9889-66c35458286a.png) @@ -38,6 +38,8 @@ If a new review has been started locally, it appears at the top of the "Reviews" * Choosing "Cancel" dismisses the review and any comments made. If there are local review comments that will be lost, a confirmation prompt is shown first. * Choosing "Submit review" submits the drafted review to GitHub. +If there is no pending review, the tile instead displays a control to create one. + ### IssueishDetailItem Each `IssueishDetailItem` opened on a pull request displays the full, multi-file diff associated with the pull request. From a7b7bae3004bf184ea52a9009264fd4345b2f61f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 3 Oct 2018 00:53:53 -0700 Subject: [PATCH 29/38] Update RFC to reflect editor-first design Co-Authored-By: simurai --- docs/rfcs/XXX-pull-request-review.md | 157 ++++++++++++++++++++------- 1 file changed, 116 insertions(+), 41 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 95003ecec4..7fb9271b78 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -18,69 +18,126 @@ Peer review is also a critical part of the path to acceptance for pull requests ## Explanation -### Current pull request tile +### Review information in Pull Request list -Reviews on the current pull request are rendered as a list on the current pull request tile. +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 it's own section at the top of the list. -![review-list](https://user-images.githubusercontent.com/378023/46273505-b9533280-c590-11e8-840e-a8eac8023cad.png) +slack_-_github +Note: Change "Current pull request" -* The review summary bubble is elided after the first sentence or N characters if necessary. -* Clicking the review summary bubble opens an `IssueishDetailItem` in the workspace center. -* Clicking a line comment opens or activates an editor on the referenced file and scrolls to center the comment's line, translated according to local changes if appropriate. -* Line comments within the review are rendered: _with a vertical blue bar_ before the file has been opened and the corresponding decoration is visible; _with a vertical grey bar_ after the file and decoration have been seen; and _with a vertical green bar_ after the comment has been marked "resolved" with the control on its decoration. -* The review summary bubble and line comment lists are greyed out if a different `IssueishDetailItem` is activated. +Clicking a pull request in the list opens a `PullRequestDetailItem` in the workspace center. -If a pending review is present, it appears at the top of the "Reviews" section within this tile: +For PRs that are not listed in the panel, users can use the `github:open-issue-or-pull-request` command: -![pending-review](https://user-images.githubusercontent.com/378023/46275946-9bd69680-c599-11e8-9889-66c35458286a.png) +xxx-pull-request-review_md_ ___github_github + + +### PullRequestDetailItem + +Each `PullRequestDetailItem` opened on a pull request displays the full, multi-file diff associated with the pull request. Review comments are shown within the diff. See ["Comment decorations"](#comment-decorations) for description of review comments. + +![screen shot 2018-10-03 at 1 50 18 pm](https://user-images.githubusercontent.com/7910250/46391711-1df6b600-c693-11e8-87f3-ad4cdbe8ebd8.png) + +TODO: update mock to have "Start review button" and "Add single comment" + +Diffs are editable ONLY if the pull request branch is checked out and the local branch history has not diverged from the remote branch history. Otherwise diffs are not editable. + +A panel at the bottom of the pane offers various options for sorting and filtering the diff. It also has a "Review Changes" button. + +#### Sort Options + +slack_-_github + +The default view is sorted by files. This is akin to the "Files changed" tab on dotcom. It displays the diff for all changed files in the PR. + +Sorting by reviews is akin to the review summaries that appear on the "Conversation" tab on dotcom. The comments are displayed grouped by review along with some context lines. + +![screen shot 2018-10-03 at 1 50 08 pm](https://user-images.githubusercontent.com/7910250/46394598-6ebfdc00-c69e-11e8-84eb-39ccbcccf736.png) + +TODO: include show multiple reviews stacked + +Sorting by commits is akin to the "Commits" tab on dotcom. A list of commits is displayed in chronological order, oldest commit on top. Clicking a commit expands the diff contents below. If there is a commit message body this is displayed as well. + +TODO: include commit sort mockup + +A banner at the bottom of the pane offers navigation to individual files within the diff and to individual review comments, allows each review to be hidden or shown with a filter control, and shows a progress bar that counts "resolved" review comments. The banner remains visible as you scroll the pane. + +#### Filter Options + +slack_-_github + +The default is to show all files, all authors, and unresolved comments. + +Filtering based on file type limits the diff view to displaying only that file type. + +TODO: Consider adding a "Find" input field that allows us to filter based on search term (which could be a file name, an author, a variable name, etc). Probably out of scope for this RFC. + +Clicking an author's avatar displays only their review information. + +Clicking "unresolved" shows only resolved comments, helping users stay focused on comments that need to be addressed. + +Clicking "resolved" shows only resolved comments. This allows users to quickly see what has already been addressed. + +Checking "all comments" shows both resolved and unresolved comments. + +Clicking "none" hides all comments, in the event that users want to see diff information only. + +#### Submitting a Review + +slack_-_github + +Clicking the "Review Changes" button reveals a UI much like dotcom's + +xxx-pull-request-review_md_ ___github_github + +TODO: update Review Changes mockup * The review summary is a TextEditor that may be used to compose a summary comment. * Choosing "Cancel" dismisses the review and any comments made. If there are local review comments that will be lost, a confirmation prompt is shown first. * Choosing "Submit review" submits the drafted review to GitHub. -If there is no pending review, the tile instead displays a control to create one. +#### Summary Box -### IssueishDetailItem +At the top of the pane is the existing summary box: -Each `IssueishDetailItem` opened on a pull request displays the full, multi-file diff associated with the pull request. +issueish-detail-item pane +TODO: add conversation/timeline icon and progress bar -A banner at the top of the pane offers navigation to individual files within the diff and to individual review comments, allows each review to be hidden or shown with a filter control, and shows a progress bar that counts "resolved" review comments. The banner remains visible as you scroll the pane. +Clicking on the "22 commits" opens the commit view and changes the bottom panel to indicate sort by commits. -At the top of the pane is the existing summary box: +Clicking on the "1 changed files" opens the files view and changes the bottom panel to indicate sort by files and "all files" checked. -issueish-detail-item pane +Clicking on the build status summary icon (green checkmark, donut chart, or X) expands an ephemeral panel beneath the summary box showing build review status. Clicking the icon again or clicking on "dismiss" dismisses it. -* Clicking on the build status summary icon (green checkmark, donut chart, or X) expands an ephemeral panel beneath the summary box showing build review status. Clicking the icon again or clicking on "dismiss" dismisses it. -* Clicking on the commit count opens the log view to those commits. +slack_-_github -Summary comments for each existing review appear in a list below that. If a pending review is being drafted, it appears at the end of the list; otherwise, a control is present to create one. A pending review may be finalized by submitting a form that appears adjacent to it. +Clicking on the conversation/timeline icon expands an ephemeral panel beneath the summary box showing a very timeline view. The PR description and PR comments are displayed here. Other note-worthy timeline events are displayed in a very minimal fashion. -After the summary comments, the diff is shown, with review comments in place: +TODO: add conversation/timeline popover mockup -![changes-tab](https://user-images.githubusercontent.com/378023/46287431-6e9bdf80-c5bd-11e8-99eb-f3f81ba64e81.png) +Clicking the "expand" icon on the top right opens this information in a new pane to the right for easy side-by-side viewing with the diff (much like our current markdown preview opens in a separate pane). -On each review comment decoration: +TODO: add conversation/timeline pane item -* The up and down arrow buttons quickly scroll to center the next or previous comment within this tab. -* Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. If the current pull request is not checked out, the "code" button is disabled, and a tooltip prompts the user to check out the pull request to edit the source. -* Reaction emoji may be added to each comment with the "emoji" button. Existing emoji reaction tallies are included beneath each comment. -* Clicking "mark as resolved" marks the comment as resolved with on GitHub. If the "reply..." editor has non-whitespace content, it is submitted as a final comment first. -* The "comment" button is disabled unless the "reply" editor is expanded and has non-whitespace content. -* Clicking "comment" submits the response as a new stand-alone comment on that thread. +Clicking on the a commit takes you to the commit view and expands the selected commit, centering it in view. + +TODO: add commit view mockup -Hovering in the diff's gutter reveals a `+` icon that allows users to begin creating a new pending review, or making an isolated comment, using the same UI described in ["In-editor decorations"](#in-editor-decorations). If a pending review is present, its comments are also shown and editable here. +Clicking on a review reference takes you to the review view and expands the selected review, centering it in view. -### In-editor decorations +TODO: add review mockup -When opening a TextEditor on a file that has been annotated with review comments on the current pull request, a block decoration is used to show the comment content at the corresponding position within the file content. Also, a gutter decoration is used to reveal lines that are included within the current pull requests' diff and may therefore include comments. -![in-editor](https://user-images.githubusercontent.com/378023/44790482-69bcc800-abda-11e8-8a0f-922c0942b8c6.png) +### Comment decorations -> TODO: add gutter decoration? +Within the multi-file diff view, a block decoration is used to show the comment content at the corresponding position within the file content. * The comment's position is calculated from the position acquired by the GitHub API response, modified based on the git diff of that file (following renames) between the owning review's attached commit and the current state of the working copy (including any local modifications). Once created, the associated marker will also track unsaved modifications to the file in real time. -* The up and down arrow buttons navigate to the next and previous review comments within this review within their respective TextEditors. -* The "diff" button navigates to the corresponding pull request's `IssueishDetailItem` and scrolls to center the same comment within that view. +* The up and down arrow buttons navigate to the next and previous review comments. +* Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the line corresponding to the comment. + * In the future we will implement inline review comments as decorations in the code. + * If the current pull request is not checked out, the "code" button is disabled, and a tooltip prompts the user to check out the pull request to edit the source. +* Reaction emoji may be added to each comment with the "emoji" button. Existing emoji reaction tallies are included beneath each comment. Hovering along the gutter within a pull request diff region reveals a `+` icon, which may be clicked to begin a new review: @@ -91,25 +148,38 @@ Clicking the `+` reveals a new comment box, which may be used to submit a single ![single-review](https://user-images.githubusercontent.com/378023/40351475-78a527c2-5de7-11e8-8006-72d859514ecc.png) * If a draft review is already in progress, the "Add single comment" button is disabled and the "Start a review" button reads "Add review comment". -* Clicking "Add single comment" submits a non-review diff comment and does not create a draft review. -* Clicking "Start a review" creates a draft review and attaches the authored comment to it. +* Clicking "Add single comment" submits a non-review diff comment and does not create a draft review. This button is disabled unless the "reply" editor is expanded and has non-whitespace content. +* Clicking "Start a review" creates a draft review and attaches the authored comment to it. This button is disabled unless the "reply" editor is expanded and has non-whitespace content. +* Clicking "mark as resolved" marks the comment as resolved with on GitHub. If the "reply..." editor has non-whitespace content, it is submitted as a final comment first. ## Drawbacks This adds a substantial amount of complexity to the UI, which is only justified for users that use GitHub pull request reviews. -Showing all reviews in the current pull request tile can easily overwhelm the other pull request information included there. It also limits our ability to expand the information we provide there in the future (like associated issues, say). - Rendering pull request comments within TextEditors can be intrusive: if there are many, or if your reviewers are particularly verbose, they could easily crowd out the code that you're trying to write and obscure your context. ## Rationale and alternatives +Our original design looked and felt very dotcom-esque: + +![changes-tab](https://user-images.githubusercontent.com/378023/46287431-6e9bdf80-c5bd-11e8-99eb-f3f81ba64e81.png) + +We decided to switch to an editor-first approach and build the code review experience around an actual TextEditor item with a custom diff view. We are breaking free of the dotcom paradigm and leveraging the fact that we are in the context of the user's working directory, where we can easily update code. + +We discussed displaying review summary information in the GitHub panel in a ["Current pull request tile"](https://github.com/atom/github/blob/2ab74b59873c3b5bccac7ef679795eb483b335cf/docs/rfcs/XXX-pull-request-review.md#current-pull-request-tile). The current design encapsulates all of the PR information and functionality within a `PullRequestDetailItem`. Keeping the GitHub panel free of PR details for a specific PR rids us of the problem of having to keep it updated when the user switches active repos (which can feel jarring). This also avoids confusing the user by showing PR details for different PRs (imagine the checked out PR info in the panel and a pane item with PR info for a separate repo). We also free up space in the GitHub panel, making it less busy/overwhelming and leaving room for other information we might want to provide there in the future (like associated issues, say). + +We also discussed implementing comment decorations in regular text editors. Clicking the "code" (<>) button on a comment was originally to take you to the comment in the corresponding TextEditor file. While this is a nice feature to have, we can ship a complete code review experience without it. Let's punt on this and tackle it later on. + ## Unresolved questions ### Questions I expect to address before this is merged +When there are working directory changes, how do we clearly indicate them within the diff view? Do we need to make them visually distinct from the PR changes? Things might get confusing for the user when the diff in the editor gets out of sync with the diff on dotcom. +Example: +* Author reads comment pointing out typo in an added line. Author edits text in multi-file diff which modifies the working directory. Should this line now be orange to indicate that it has deviated from the original diff? + Can we access "draft" reviews from the GitHub API, to unify them between Atom and GitHub? * _Yes, the `reviews` object includes it in a `PENDING` state._ @@ -127,6 +197,8 @@ Similarly, are there any ways we can encourage empathy within the review authori * _Emoji reactions on comments :cake: :tada:_ * _Enable integration with Teletype for smoother jumping to a synchronous review_ +How do we clearly indicating recently added changes? That is, new changes pushed, comments, reviews, etc since the last time the users viewed the PR info. Is it enough to simply update the timeline view? Is it too easy to miss changes? + ### Questions I expect to resolve throughout the implementation process Review comment positioning within live TextEditors will be a tricky problem to address satisfactorily. What are the edge cases we need to handle there? @@ -142,10 +214,13 @@ How do we handle comment threads? What other pull request information can we add to the GitHub pane item? -Are there other tabs that we need within the `IssueishDetailItem`? - How can we notify users when new information, including reviews, is available, preferably without being intrusive or disruptive? ## Implementation phases ![dependency-graph](https://user-images.githubusercontent.com/17565/46361100-d47a7c80-c63a-11e8-83de-4a548be9cb9c.png) + +## Related features out of scope of this RFC + +* Inline review comments +* "Find" input field for filtering based on search term (which could be a file name, an author, a variable name, etc) From cd41db432dd84aba4b2557590c58c66e3dbdab38 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 3 Oct 2018 17:14:59 -0700 Subject: [PATCH 30/38] Incorporate feedback Co-Authored-By: Ash Wilson Co-Authored-By: Tilde Ann Thurium --- docs/rfcs/XXX-pull-request-review.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 7fb9271b78..83ba5d69cb 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -23,7 +23,7 @@ Peer review is also a critical part of the path to acceptance for pull requests 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 it's own section at the top of the list. slack_-_github -Note: Change "Current pull request" +Note: Change "Current pull request" to "Checked out pull request" Clicking a pull request in the list opens a `PullRequestDetailItem` in the workspace center. @@ -40,7 +40,7 @@ Each `PullRequestDetailItem` opened on a pull request displays the full, multi-f TODO: update mock to have "Start review button" and "Add single comment" -Diffs are editable ONLY if the pull request branch is checked out and the local branch history has not diverged from the remote branch history. Otherwise diffs are not editable. +Diffs are editable ONLY if the pull request branch is checked out and the local branch history has not diverged from the remote branch history. Otherwise diffs are not editable. Details of this will be fleshed out in a separate RFC specifically for editable diffs. A panel at the bottom of the pane offers various options for sorting and filtering the diff. It also has a "Review Changes" button. @@ -48,6 +48,8 @@ A panel at the bottom of the pane offers various options for sorting and filteri slack_-_github +Note: We probably want to find better verbiage than "sort". Let's also consider a dropdown menu UX to select different views of the data. + The default view is sorted by files. This is akin to the "Files changed" tab on dotcom. It displays the diff for all changed files in the PR. Sorting by reviews is akin to the review summaries that appear on the "Conversation" tab on dotcom. The comments are displayed grouped by review along with some context lines. @@ -56,7 +58,7 @@ Sorting by reviews is akin to the review summaries that appear on the "Conversat TODO: include show multiple reviews stacked -Sorting by commits is akin to the "Commits" tab on dotcom. A list of commits is displayed in chronological order, oldest commit on top. Clicking a commit expands the diff contents below. If there is a commit message body this is displayed as well. +Sorting by commits is akin to the "Commits" tab on dotcom. A list of commits is displayed in chronological order, oldest commit on top. Clicking a commit expands the diff contents below. If there is a commit message body this is displayed as well. Commit diffs are not editable. TODO: include commit sort mockup @@ -111,7 +113,7 @@ Clicking on the build status summary icon (green checkmark, donut chart, or X) e slack_-_github -Clicking on the conversation/timeline icon expands an ephemeral panel beneath the summary box showing a very timeline view. The PR description and PR comments are displayed here. Other note-worthy timeline events are displayed in a very minimal fashion. +Clicking on the conversation/timeline icon expands an ephemeral panel beneath the summary box showing a very timeline view. The PR description and PR comments are displayed here. Other note-worthy timeline events are displayed in a very minimal fashion. At the bottom is an input field to add a new PR comment. TODO: add conversation/timeline popover mockup @@ -178,7 +180,7 @@ We also discussed implementing comment decorations in regular text editors. Clic When there are working directory changes, how do we clearly indicate them within the diff view? Do we need to make them visually distinct from the PR changes? Things might get confusing for the user when the diff in the editor gets out of sync with the diff on dotcom. Example: -* Author reads comment pointing out typo in an added line. Author edits text in multi-file diff which modifies the working directory. Should this line now be orange to indicate that it has deviated from the original diff? +* Author reads comment pointing out typo in an added line. Author edits text in multi-file diff which modifies the working directory. Should this line now be styled differently to indicate that it has deviated from the original diff? Can we access "draft" reviews from the GitHub API, to unify them between Atom and GitHub? @@ -197,8 +199,6 @@ Similarly, are there any ways we can encourage empathy within the review authori * _Emoji reactions on comments :cake: :tada:_ * _Enable integration with Teletype for smoother jumping to a synchronous review_ -How do we clearly indicating recently added changes? That is, new changes pushed, comments, reviews, etc since the last time the users viewed the PR info. Is it enough to simply update the timeline view? Is it too easy to miss changes? - ### Questions I expect to resolve throughout the implementation process Review comment positioning within live TextEditors will be a tricky problem to address satisfactorily. What are the edge cases we need to handle there? From 29c6a6e25e1e25a846386870a5a793e528f15d7b Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 3 Oct 2018 17:19:07 -0700 Subject: [PATCH 31/38] Add in-line comments back Co-Authored-By: Ash Wilson --- docs/rfcs/XXX-pull-request-review.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 83ba5d69cb..f00b165089 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -136,8 +136,7 @@ Within the multi-file diff view, a block decoration is used to show the comment * The comment's position is calculated from the position acquired by the GitHub API response, modified based on the git diff of that file (following renames) between the owning review's attached commit and the current state of the working copy (including any local modifications). Once created, the associated marker will also track unsaved modifications to the file in real time. * The up and down arrow buttons navigate to the next and previous review comments. -* Clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the line corresponding to the comment. - * In the future we will implement inline review comments as decorations in the code. +* For comment decorations in the `PullRequestDetailItem`, clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. * If the current pull request is not checked out, the "code" button is disabled, and a tooltip prompts the user to check out the pull request to edit the source. * Reaction emoji may be added to each comment with the "emoji" button. Existing emoji reaction tallies are included beneath each comment. @@ -170,8 +169,6 @@ We decided to switch to an editor-first approach and build the code review exper We discussed displaying review summary information in the GitHub panel in a ["Current pull request tile"](https://github.com/atom/github/blob/2ab74b59873c3b5bccac7ef679795eb483b335cf/docs/rfcs/XXX-pull-request-review.md#current-pull-request-tile). The current design encapsulates all of the PR information and functionality within a `PullRequestDetailItem`. Keeping the GitHub panel free of PR details for a specific PR rids us of the problem of having to keep it updated when the user switches active repos (which can feel jarring). This also avoids confusing the user by showing PR details for different PRs (imagine the checked out PR info in the panel and a pane item with PR info for a separate repo). We also free up space in the GitHub panel, making it less busy/overwhelming and leaving room for other information we might want to provide there in the future (like associated issues, say). -We also discussed implementing comment decorations in regular text editors. Clicking the "code" (<>) button on a comment was originally to take you to the comment in the corresponding TextEditor file. While this is a nice feature to have, we can ship a complete code review experience without it. Let's punt on this and tackle it later on. - ## Unresolved questions From ea3973bdac7202f69433f5c8ca7faae4cad80911 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 4 Oct 2018 08:25:21 -0400 Subject: [PATCH 32/38] Mark remaining TODOs as :construction: --- docs/rfcs/XXX-pull-request-review.md | 54 +++++++++++++--------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index f00b165089..f5c41a4dd0 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -22,23 +22,21 @@ Peer review is also a critical part of the path to acceptance for pull requests 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 it's own section at the top of the list. -slack_-_github -Note: Change "Current pull request" to "Checked out pull request" +pull request list with review progress bars Clicking a pull request in the list opens a `PullRequestDetailItem` in the workspace center. For PRs that are not listed in the panel, users can use the `github:open-issue-or-pull-request` command: -xxx-pull-request-review_md_ ___github_github - +opening a pull request by URL ### PullRequestDetailItem Each `PullRequestDetailItem` opened on a pull request displays the full, multi-file diff associated with the pull request. Review comments are shown within the diff. See ["Comment decorations"](#comment-decorations) for description of review comments. -![screen shot 2018-10-03 at 1 50 18 pm](https://user-images.githubusercontent.com/7910250/46391711-1df6b600-c693-11e8-87f3-ad4cdbe8ebd8.png) +![pull request detail item](https://user-images.githubusercontent.com/7910250/46391711-1df6b600-c693-11e8-87f3-ad4cdbe8ebd8.png) -TODO: update mock to have "Start review button" and "Add single comment" +> :construction: Update mock to have "Start review button" and "Add single comment" Diffs are editable ONLY if the pull request branch is checked out and the local branch history has not diverged from the remote branch history. Otherwise diffs are not editable. Details of this will be fleshed out in a separate RFC specifically for editable diffs. @@ -46,33 +44,33 @@ A panel at the bottom of the pane offers various options for sorting and filteri #### Sort Options -slack_-_github +sort by -Note: We probably want to find better verbiage than "sort". Let's also consider a dropdown menu UX to select different views of the data. +> :construction: We probably want to find better verbiage than "sort". Let's also consider a dropdown menu UX to select different views of the data. The default view is sorted by files. This is akin to the "Files changed" tab on dotcom. It displays the diff for all changed files in the PR. Sorting by reviews is akin to the review summaries that appear on the "Conversation" tab on dotcom. The comments are displayed grouped by review along with some context lines. -![screen shot 2018-10-03 at 1 50 08 pm](https://user-images.githubusercontent.com/7910250/46394598-6ebfdc00-c69e-11e8-84eb-39ccbcccf736.png) +![sorted by reviews](https://user-images.githubusercontent.com/7910250/46394598-6ebfdc00-c69e-11e8-84eb-39ccbcccf736.png) -TODO: include show multiple reviews stacked +> :construction: Show multiple reviews stacked Sorting by commits is akin to the "Commits" tab on dotcom. A list of commits is displayed in chronological order, oldest commit on top. Clicking a commit expands the diff contents below. If there is a commit message body this is displayed as well. Commit diffs are not editable. -TODO: include commit sort mockup +> :construction: Include commit sort mockup A banner at the bottom of the pane offers navigation to individual files within the diff and to individual review comments, allows each review to be hidden or shown with a filter control, and shows a progress bar that counts "resolved" review comments. The banner remains visible as you scroll the pane. #### Filter Options -slack_-_github +filter options The default is to show all files, all authors, and unresolved comments. Filtering based on file type limits the diff view to displaying only that file type. -TODO: Consider adding a "Find" input field that allows us to filter based on search term (which could be a file name, an author, a variable name, etc). Probably out of scope for this RFC. +> :construction: Consider adding a "Find" input field that allows us to filter based on search term (which could be a file name, an author, a variable name, etc). Clicking an author's avatar displays only their review information. @@ -86,13 +84,13 @@ Clicking "none" hides all comments, in the event that users want to see diff inf #### Submitting a Review -slack_-_github +review changes button -Clicking the "Review Changes" button reveals a UI much like dotcom's +Clicking the "Review Changes" button reveals a UI much like dotcom's: -xxx-pull-request-review_md_ ___github_github +review changes panel -TODO: update Review Changes mockup +> :construction: Update the Review Changes mockup * The review summary is a TextEditor that may be used to compose a summary comment. * Choosing "Cancel" dismisses the review and any comments made. If there are local review comments that will be lost, a confirmation prompt is shown first. @@ -102,8 +100,9 @@ TODO: update Review Changes mockup At the top of the pane is the existing summary box: -issueish-detail-item pane -TODO: add conversation/timeline icon and progress bar +pull request details pane summary box + +> :construction: Add conversation/timeline icon and progress bar Clicking on the "22 commits" opens the commit view and changes the bottom panel to indicate sort by commits. @@ -111,24 +110,23 @@ Clicking on the "1 changed files" opens the files view and changes the bottom pa Clicking on the build status summary icon (green checkmark, donut chart, or X) expands an ephemeral panel beneath the summary box showing build review status. Clicking the icon again or clicking on "dismiss" dismisses it. -slack_-_github +emphemeral checks panel Clicking on the conversation/timeline icon expands an ephemeral panel beneath the summary box showing a very timeline view. The PR description and PR comments are displayed here. Other note-worthy timeline events are displayed in a very minimal fashion. At the bottom is an input field to add a new PR comment. -TODO: add conversation/timeline popover mockup +> :construction: Add conversation/timeline popover mockup Clicking the "expand" icon on the top right opens this information in a new pane to the right for easy side-by-side viewing with the diff (much like our current markdown preview opens in a separate pane). -TODO: add conversation/timeline pane item +> :construction: Add conversation/timeline pane item Clicking on the a commit takes you to the commit view and expands the selected commit, centering it in view. -TODO: add commit view mockup +> :construction: Add commit view mockup Clicking on a review reference takes you to the review view and expands the selected review, centering it in view. -TODO: add review mockup - +> :construction: Add review mockup ### Comment decorations @@ -169,8 +167,6 @@ We decided to switch to an editor-first approach and build the code review exper We discussed displaying review summary information in the GitHub panel in a ["Current pull request tile"](https://github.com/atom/github/blob/2ab74b59873c3b5bccac7ef679795eb483b335cf/docs/rfcs/XXX-pull-request-review.md#current-pull-request-tile). The current design encapsulates all of the PR information and functionality within a `PullRequestDetailItem`. Keeping the GitHub panel free of PR details for a specific PR rids us of the problem of having to keep it updated when the user switches active repos (which can feel jarring). This also avoids confusing the user by showing PR details for different PRs (imagine the checked out PR info in the panel and a pane item with PR info for a separate repo). We also free up space in the GitHub panel, making it less busy/overwhelming and leaving room for other information we might want to provide there in the future (like associated issues, say). - - ## Unresolved questions ### Questions I expect to address before this is merged @@ -185,11 +181,11 @@ Can we access "draft" reviews from the GitHub API, to unify them between Atom an How do we represent the resolution of a comment thread? Where can we reveal this progress through each review, and of all required reviews? -* _We'll show a progress bar on a sticky header at the top of the `IssueishDetailItem`._ +* _We'll show a progress bar on a sticky header at the top of the `PullRequestDetailItem`._ Are there any design choices we can make to lessen the emotional weight of a "requests changes" review? Peer review has the most value when it discovers issues for the pull request author to address, but accepting criticism is a vulnerable moment. -* _Chosing phrasing and iconography carefully for "recommend changes"._ +* _Choosing phrasing and iconography carefully for "recommend changes"._ Similarly, are there any ways we can encourage empathy within the review authoring process? Can we encourage reviewers to make positive comments or demonstrate humility and open-mindedness? From 2750281c934e88f21a38443b40a2979533804d8d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 4 Oct 2018 08:37:38 -0400 Subject: [PATCH 33/38] Add an Explanation section for in-editor comment rendering --- docs/rfcs/XXX-pull-request-review.md | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index f5c41a4dd0..248d18418c 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -128,17 +128,32 @@ Clicking on a review reference takes you to the review view and expands the sele > :construction: Add review mockup +### In-editor decorations + +When opening a TextEditor on a file that has been annotated with review comments on the current pull request, a block decoration is used to show the comment content at the corresponding position within the file content. Also, a gutter decoration is used to reveal lines that are included within the current pull requests' diff and may therefore include comments. + +![in-editor review comment decoration](https://user-images.githubusercontent.com/378023/44790482-69bcc800-abda-11e8-8a0f-922c0942b8c6.png) + +> :construction: Add gutter decoration? + +* The comment's position is calculated from the position acquired by the GitHub API response, modified based on the git diff of that file (following renames) between the owning review's attached commit and the current state of the working copy (including any local modifications). Once created, the associated marker will also track unsaved modifications to the file in real time. +* The up and down arrow buttons navigate to the next and previous review comments within this review within their respective TextEditors. +* The "diff" button navigates to the corresponding pull request's detail item and scrolls to center the same comment within that view. + ### Comment decorations -Within the multi-file diff view, a block decoration is used to show the comment content at the corresponding position within the file content. +Within the multi-file diff view or a TextEditor, a block decoration is used to show the comment content at the corresponding position within the file content. * The comment's position is calculated from the position acquired by the GitHub API response, modified based on the git diff of that file (following renames) between the owning review's attached commit and the current state of the working copy (including any local modifications). Once created, the associated marker will also track unsaved modifications to the file in real time. * The up and down arrow buttons navigate to the next and previous review comments. * For comment decorations in the `PullRequestDetailItem`, clicking the "code" (`<>`) button opens the corresponding file in a TextEditor and scrolls to the review comment decoration there. * If the current pull request is not checked out, the "code" button is disabled, and a tooltip prompts the user to check out the pull request to edit the source. +* For comment decorations within a `TextEditor`, clicking the "diff" button opens the corresponding `PullRequestDetailItem` and scrolls to focus the equivalent comment. * Reaction emoji may be added to each comment with the "emoji" button. Existing emoji reaction tallies are included beneath each comment. -Hovering along the gutter within a pull request diff region reveals a `+` icon, which may be clicked to begin a new review: +### Line comment creation + +Hovering along the gutter within a pull request diff region in a `TextEditor` or a `PullRequestDetailItem` reveals a `+` icon, which may be clicked to begin a new review: ![plus-icon](https://user-images.githubusercontent.com/378023/40348708-6698b2ea-5ddf-11e8-8eaa-9d95bc483fb1.png) From c44566882bb37e7dcbd3eb91527637f48826b05a Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 4 Oct 2018 08:45:39 -0400 Subject: [PATCH 34/38] Move diff editability questions to "answered during implementation" --- docs/rfcs/XXX-pull-request-review.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 248d18418c..11ca6574a0 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -38,7 +38,7 @@ Each `PullRequestDetailItem` opened on a pull request displays the full, multi-f > :construction: Update mock to have "Start review button" and "Add single comment" -Diffs are editable ONLY if the pull request branch is checked out and the local branch history has not diverged from the remote branch history. Otherwise diffs are not editable. Details of this will be fleshed out in a separate RFC specifically for editable diffs. +Diffs are editable _only_ if the pull request branch is checked out and the local branch history has not diverged from the remote branch history. A panel at the bottom of the pane offers various options for sorting and filtering the diff. It also has a "Review Changes" button. @@ -218,6 +218,11 @@ The GraphQL API paths we need to interact with all involve multiple levels of pa How do we handle comment threads? +When editing diffs: + +* Do we edit the underlying buffer or file directly, or do we mark the `PullRequestDetailItem` as "modified" and require a "save" action to persist changes? +* Do we disallow edits of removed lines, or do we re-introduce the removed line as an addition on modification? + ### Questions I consider out of scope of this RFC What other pull request information can we add to the GitHub pane item? @@ -230,5 +235,4 @@ How can we notify users when new information, including reviews, is available, p ## Related features out of scope of this RFC -* Inline review comments * "Find" input field for filtering based on search term (which could be a file name, an author, a variable name, etc) From 17ec9370cde9baa7437fd655f6af54c5d2af160e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 4 Oct 2018 08:49:29 -0400 Subject: [PATCH 35/38] Move this question from "before merge" to "during implementation" aka "cheating" --- docs/rfcs/XXX-pull-request-review.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 11ca6574a0..36b1a95ddd 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -186,10 +186,6 @@ We discussed displaying review summary information in the GitHub panel in a ["Cu ### Questions I expect to address before this is merged -When there are working directory changes, how do we clearly indicate them within the diff view? Do we need to make them visually distinct from the PR changes? Things might get confusing for the user when the diff in the editor gets out of sync with the diff on dotcom. -Example: -* Author reads comment pointing out typo in an added line. Author edits text in multi-file diff which modifies the working directory. Should this line now be styled differently to indicate that it has deviated from the original diff? - Can we access "draft" reviews from the GitHub API, to unify them between Atom and GitHub? * _Yes, the `reviews` object includes it in a `PENDING` state._ @@ -209,6 +205,8 @@ Similarly, are there any ways we can encourage empathy within the review authori ### Questions I expect to resolve throughout the implementation process +When there are working directory changes or local commits on the PR branch, how do we clearly indicate them within the diff view? Do we need to make them visually distinct from the PR changes? Things might get confusing for the user when the diff in the editor gets out of sync with the diff on dotcom. For example: a pull request author reads a comment pointing out a typo in an added line. The author edits text within the multi-file diff which modifies the working directory. Should this line now be styled differently to indicate that it has deviated from the original diff? + Review comment positioning within live TextEditors will be a tricky problem to address satisfactorily. What are the edge cases we need to handle there? * _Review comments on deleted lines._ From e6c34abfaa8907aa8da00c250a78c33311b834a7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 4 Oct 2018 09:01:17 -0400 Subject: [PATCH 36/38] Dependency graph bump --- docs/rfcs/XXX-pull-request-review.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/XXX-pull-request-review.md index 36b1a95ddd..ff981eae1e 100644 --- a/docs/rfcs/XXX-pull-request-review.md +++ b/docs/rfcs/XXX-pull-request-review.md @@ -229,7 +229,7 @@ How can we notify users when new information, including reviews, is available, p ## Implementation phases -![dependency-graph](https://user-images.githubusercontent.com/17565/46361100-d47a7c80-c63a-11e8-83de-4a548be9cb9c.png) +![dependency-graph](https://user-images.githubusercontent.com/17565/46475622-019e6a80-c7b4-11e8-9bf5-8223d5c6631f.png) ## Related features out of scope of this RFC From bad57704f9f9b86e1599088f3211bc68aef02a3e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 4 Oct 2018 09:21:54 -0400 Subject: [PATCH 37/38] Give it a number :tada: --- .../{XXX-pull-request-review.md => 003-pull-request-review.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename docs/rfcs/{XXX-pull-request-review.md => 003-pull-request-review.md} (100%) diff --git a/docs/rfcs/XXX-pull-request-review.md b/docs/rfcs/003-pull-request-review.md similarity index 100% rename from docs/rfcs/XXX-pull-request-review.md rename to docs/rfcs/003-pull-request-review.md From 5ad24e6037e94b2c976fbefa418691df856c478f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 4 Oct 2018 09:22:55 -0400 Subject: [PATCH 38/38] Status: accepted :hammer: --- docs/rfcs/003-pull-request-review.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/rfcs/003-pull-request-review.md b/docs/rfcs/003-pull-request-review.md index ff981eae1e..06eb3bd368 100644 --- a/docs/rfcs/003-pull-request-review.md +++ b/docs/rfcs/003-pull-request-review.md @@ -1,10 +1,8 @@ -# Feature title - -Pull Request Review +# Pull Request Review ## Status -Proposed +Accepted ## Summary