RFC for Pull Request Review III #1725
Conversation
Since there aren't any changes.
simurai
left a comment
simurai
left a comment
There was a problem hiding this comment.
Two questions that came up...
- Now that "Suggested changes" got announced at Universe, should we already start thinking how to incorporate that in this RFC?
- If we do everything in this RFC at once, it would be a gigantic PR. How can we break it up into smaller implementation phases so that we can keep adding value with smaller ships?
I think it makes sense to keep suggested changes out of this RFC so that the scope of work stays focused. |
smashwilson
left a comment
smashwilson
left a comment
There was a problem hiding this comment.
I'm 👍 on these design changes, modulo a few comments for discussion 😄
|
|
||
| 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. | ||
| - Overview | ||
| - Files (**new**) |
There was a problem hiding this comment.
Did we still want Files to be the default tab, to feel more "editor-y"?
There was a problem hiding this comment.
👍 Although then we probably should rename "Overview" to "Conversation" or similar.
| ##### `+` Button | ||
|
|
||
| Clicking "none" hides all comments, in the event that users want to see diff information only. | ||
| Hovering along the gutter within a pull request diff region in a `TextEditor` or a `PullRequestDetailItem` reveals a `+` icon. Clicking the `+` icon reveals a new comment box, which may be used to submit a single comment or start a multi-comment review: |
There was a problem hiding this comment.
We can only create new review comments at certain locations within the files, though: it has to be within a diff region on the PR. Can we add some kind of gutter decoration to signal where these regions are?
|
Just adding @kuychaco's point from the design review before I forget it:
Example:
|
|
@simurai How would you feel about merging this as-is? It's a bit confusing to have the currently-definitive form of this feature request here instead of in |
|
Sure. 👍 I guess we can make an exception here and not wait until it's shipped. This one is more like a "longer term" RFC. |
It's less that and more that we already merged a version of it, so it feels like we should keep that as the "canon" version we're shooting for. Feature requests we create from this point on will follow the "keep the PR open" style (until we change our minds about it I guess). |
This is a 3rd iteration on "Pull Request Reviews" in Atom.
✨ Rendered ✨
🚶♀️ Walkthrough 🚶
🏓 Static prototype 🦗
Summary of the changes:
History:
Version 2: #1706
Version 1: #1712