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

Commit cd41db4

Browse files
kuychacosmashwilsonTilde Ann Thurium
committed
Incorporate feedback
Co-Authored-By: Ash Wilson <[email protected]> Co-Authored-By: Tilde Ann Thurium <[email protected]>
1 parent a7b7bae commit cd41db4

File tree

1 file changed

+7
-7
lines changed

1 file changed

+7
-7
lines changed

docs/rfcs/XXX-pull-request-review.md

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ Peer review is also a critical part of the path to acceptance for pull requests
2323
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.
2424

2525
<img width="339" alt="slack_-_github" src="https://user-images.githubusercontent.com/7910250/46391240-ad4e9a00-c690-11e8-904b-e4cfd2c0f667.png">
26-
Note: Change "Current pull request"
26+
Note: Change "Current pull request" to "Checked out pull request"
2727

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

@@ -40,14 +40,16 @@ Each `PullRequestDetailItem` opened on a pull request displays the full, multi-f
4040

4141
TODO: update mock to have "Start review button" and "Add single comment"
4242

43-
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.
43+
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.
4444

4545
A panel at the bottom of the pane offers various options for sorting and filtering the diff. It also has a "Review Changes" button.
4646

4747
#### Sort Options
4848

4949
<img width="731" alt="slack_-_github" src="https://user-images.githubusercontent.com/7910250/46392358-f6551d00-c695-11e8-8ed4-c7aa95044b06.png">
5050

51+
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.
52+
5153
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.
5254

5355
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
5658

5759
TODO: include show multiple reviews stacked
5860

59-
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.
61+
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.
6062

6163
TODO: include commit sort mockup
6264

@@ -111,7 +113,7 @@ Clicking on the build status summary icon (green checkmark, donut chart, or X) e
111113

112114
<img width="722" alt="slack_-_github" src="https://user-images.githubusercontent.com/7910250/46391893-fbb16800-c693-11e8-88e7-ffe73448f8a8.png">
113115

114-
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.
116+
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.
115117

116118
TODO: add conversation/timeline popover mockup
117119

@@ -178,7 +180,7 @@ We also discussed implementing comment decorations in regular text editors. Clic
178180

179181
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.
180182
Example:
181-
* 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?
183+
* 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?
182184

183185
Can we access "draft" reviews from the GitHub API, to unify them between Atom and GitHub?
184186

@@ -197,8 +199,6 @@ Similarly, are there any ways we can encourage empathy within the review authori
197199
* _Emoji reactions on comments :cake: :tada:_
198200
* _Enable integration with Teletype for smoother jumping to a synchronous review_
199201

200-
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?
201-
202202
### Questions I expect to resolve throughout the implementation process
203203

204204
Review comment positioning within live TextEditors will be a tricky problem to address satisfactorily. What are the edge cases we need to handle there?

0 commit comments

Comments
 (0)