New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC for Pull Request Review #1706

Merged
merged 39 commits into from Oct 4, 2018

Conversation

@smashwilson
Member

smashwilson commented Sep 27, 2018

I've started to collect the design work that @simurai, myself, and others have been collaborating on off and on over the past few months into a single document about the pull request review experience we want to support in Atom. I'm hopeful that by putting it all in one place, we can have a clearer picture about the pieces we'll need to put in place to make it a reality.

This is highly WIP - I literally just through up a few paragraphs the other day. I'm pushing it now so that all of you can see how far along I am 馃槃

/cc @kuychaco, @annthurium, @vanessayuenn, @simurai
/cc @sguthals

馃尃 馃(rendered) 馃 馃尃

@coveralls

This comment has been minimized.

coveralls commented Sep 27, 2018

Coverage Status

Coverage decreased (-0.01%) to 81.987% when pulling 5ad24e6 on aw/pr-review-rfc into 1b4c899 on master.

smashwilson added some commits Sep 27, 2018

@smashwilson

This comment has been minimized.

Member

smashwilson commented Sep 28, 2018

Okay, I think the prose here is in a state that accurately represents the contents of my head about this. I drew pretty heavily from our existing work (and used @simurai's existing excellent design graphics!) but I've also tried to "fill in the gaps" as I went.

Specifically, I've added:

  • A new row on non-current pull request tiles in the GitHub tab to show review statuses there too
  • Separate "Reviews" and "Changes" tabs within an IssueishPaneItem. "Reviews" groups pull request reviews by review; "Changes" shows review comments as well, but groups them in diff order, over the full PR diff.

And I've omitted for now:

  • Ways to discover new pull requests to review. The answer here will likely lie in enhancements and customizability of our GitHub tab accordion lists, which feels like its own effort.
  • Much discussion of how to flag new/unread reviews, or any mention of notifications. I feel like we should find an approach to these that apply uniformly to the remote data we show.
  • "Open in Atom" and preservation of state between GitHub and Atom.
  • Probably other things I just overlooked, it's a long issue 馃槄

Next, I'm going to try to do some UI mocks for the missing diagrams so you don't have to puzzle out when I mean from the text alone for the new bits. But I think this is far enough along that others could help me with reviews that:

  • Spotting gaps in functionality that I missed
  • Disagree with any of the ad-hoc UX that I put in place
  • Propose more alternatives that I didn't think of

Once we're satisfied with the state of the vision described here, we can fill out the "implementation phases" list and figure out the best places to get started 馃殌

simurai added some commits Oct 1, 2018

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.

This comment has been minimized.

@simurai

simurai Oct 1, 2018

Member
  • The icon could change from "error" to "warning".
  • Replace "Request changes" with "Recommend changes". It's less demanding, but still with a slight urge. In the end it's up to the PR author to follow the recommendation (or not).

image

image

This comment has been minimized.

@smashwilson

smashwilson Oct 1, 2018

Member

馃憤 I like that 馃槃

This comment has been minimized.

@annthurium

annthurium Oct 1, 2018

Contributor

Yeah!! Some friends of mine wrote a Chrome extension to change the "request changes" icon on dotcom to 馃檹. Changing the icon can help a lot here to improve the tone.

This comment has been minimized.

@smashwilson

smashwilson Oct 1, 2018

Member

Okay, I kind of want to use the 馃檹 emoji now...

This comment has been minimized.

@annthurium

annthurium Oct 1, 2018

Contributor

I think @simurai's icon choices above are great too! .As long as we're not using the red icon dotcom uses, which feels pretty heavy handed to me, I'm open to a wide range of options.

This comment has been minimized.

@kuychaco

kuychaco Oct 2, 2018

Member

I love this question. And I love these suggestions. This sort of discussion and exploration makes me so happy :)

### 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.

This comment has been minimized.

@simurai

simurai Oct 1, 2018

Member

Is the main purpose to have a quick way to navigate to the "Reviews" tab? Or to keep an eye on new reviews? For the later, some sort of "new comments since I last visited", might be handy, but comes with its own challenges. It also looks a bit crowded with all the icons and numbers.

This comment has been minimized.

@smashwilson

smashwilson Oct 1, 2018

Member

Is the main purpose to have a quick way to navigate to the "Reviews" tab? Or to keep an eye on new reviews?

Yes 馃槅 It's also to start using the PR result tiles to show a bit more information without having to click through, and to give a little symmetry to the information you see for current and non-current PRs.

I'm not 100% sold on it myself though mostly because:

It also looks a bit crowded with all the icons and numbers.

So if it's just too messy we can yank it.

Sidenote:

For the later, some sort of "new comments since I last visited", might be handy, but comes with its own challenges.

I'm deliberately punting anything related to notifications about new information arriving from this RFC. It's still important and a place we can add a ton of value, but I believe it's something we should address in a consistent way across the package as a whole (controls that let you opt-in to Atom notifications? Blue-bars to signal "there's something new here"?). It's also something that's going to take a lot of us getting it wrong before we figure out how to get it right.

This comment has been minimized.

@simurai

simurai Oct 2, 2018

Member

It's also to start using the PR result tiles to show a bit more information without having to click through

Maybe we could show the "reviews" progress bar?

image

And maaaaybeeee also a "tasks" bar.

non-current pull request tile

Then you can glance over the PRs and get a sense of their progress. Like " 馃槺 Oh, this looks almost ready, I better add my review."

This comment has been minimized.

@smashwilson

smashwilson Oct 2, 2018

Member

Okay, I definitely like yours better than my mockup, but I'm still not convinced we shouldn't just leave non-current PR tiles alone. I'm kind of leaning toward not touching it I guess?

@simurai

I updated a bunch of mockups, but not quite sure about the difference between "Changes" and "Reviews" tab. I guess they are for:

  • Changes tabs are for reviewers. In order to review a PR, they need to see the whole diff.
  • Reviews tab are for PR authors. A place with all reviews to respond to.

I guess that separation is nice, but maybe also confusing? Like you click on Reviews but not realizing that some of the changes (diff) are missing. Anyways, I'll sleep over it. 馃挙

@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 1, 2018

I believe both "Changes" and "Reviews" tabs provide distinct value for both scenarios. Here's the bit from the Alternative section:

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.

So basically:

User role "Changes" tab value "Reviews" tab value
PR author See unified, total PR code changes. See individual review comments with additional lines of diff context (like the whole function if it's more than a few lines). See all review comments from all reviewers, flowed together as a single conversation. See the total number of reviews. See my progress through reviews as a checklist of things to address, or at least answer, before we can get to the merge - both reviews and comments within an individual review. Read the comments within each single review as a self-contained narrative, in the order in which those comments were created. See the summary comments for each review.
Review author Create a new review. See how the review you're drafting fits in with any existing reviews on the code - has someone else already noticed something? See your pending review all together, separated from other people's comments. Double-check yourself for tone - are you leaving too many terse, negative comments? Is a lot of it redundant or repetitive? Does the summary actually summarize the stuff you wrote?

I'll also note that the "Changes" tab as written doesn't give us an easy way to show the summary comments and 馃憤 / 馃憥 associated with existing reviews or correlate a summary comment to its diff comments.

Dotcom basically mixes the "Reviews" tab into the PR timeline, and puts the checklist/progress bit in the box at the top:

screen shot 2018-10-01 at 11 05 55 am

@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 1, 2018

Also, to be clear: totally open to combining "Reviews" and "Changes" if we can. I think the only must-have that the "Reviews" tab gives us is the ability to see the review summary comments, so maybe if we can find a way to do that on "Changes"?

@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 1, 2018

... Also, @simurai, your mocks for the two tabs are almost exactly how I pictured them. You are a mindreader!

smashwilson and others added some commits Oct 2, 2018

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 <katrina@github.com>
Co-Authored-By: Sarah Guthals <sguthals@github.com>
@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 2, 2018

@kuychaco: I did my best to capture what you were proposing here, hopefully clearly enough that @simurai will know what we're describing: basically, reunify the "sub-tabs" within the IssueishDetailItem, and put the focus on the (soon to be editable?) diff.

Create a pending review if one is not present
Co-Authored-By: Sarah Guthals <sguthals@github.com>
@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 2, 2018

Okay, I've updated to include @sguthals's suggestion of permitting users to initiate new reviews from the current pull request tile or what used to be the changes view.

If there is no pending review, the tile instead displays a control to create one.
### IssueishDetailItem

This comment has been minimized.

@smashwilson

smashwilson Oct 2, 2018

Member

@kuychaco One big question based on this section. Where do we put the conversation timeline?

Update RFC to reflect editor-first design
Co-Authored-By: simurai <simurai@users.noreply.github.com>

kuychaco and others added some commits Oct 4, 2018

Incorporate feedback
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Add in-line comments back
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
Merge pull request #1712 from atom/ku/pr-review-rfc-editor-focus
Update RFC to reflect editor-first design
@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 4, 2018

I think we're all on the same page in terms of our rough design direction and scope for this work 馃帀 馃巻

Thank you everyone for weighing in and hashing this out together. Coming to consensus on something like this is really challenging and there's a super long trail of fine details that we'll hash out as we're building it. But it's already really really improved from the write-up I started at the beginning of this week, when it was just an expression of what was in my head.

Some expectations:

  • There are still a lot of 馃毀 markers in the RFC. That's okay! I strongly suspect we'll still have many of those there when we're "done." I'm certainly not intending to hold up the merge for them 馃槃
  • Our RFCs are intended to be living documents. If you wish to propose a change in direction based on new thoughts or insights gained from early implementation phases or mocks... go ahead and send a PR to change it.
  • The design captured here is meant to describe the way users may interact with the new features we're proposing at a high level, and to express a common hazy understanding of our goal. There are a million implementation details that will change once we're actually writing the code. Don't feel the need to update the RFC for every way we deviate from it, or that you have to get buy-in from everyone to make those decisions - trust your instincts, we'll catch up

smashwilson added some commits Oct 4, 2018

@smashwilson smashwilson added this to In Progress 馃敡 in Feature Sprint : 1 October - 19 November 2018 : v0.21.0 via automation Oct 4, 2018

@smashwilson smashwilson referenced this pull request Oct 4, 2018

Closed

Multi-File Diff #1689

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from In Progress 馃敡 to Merged 鈽戯笍 Oct 4, 2018

@smashwilson smashwilson merged commit b896401 into master Oct 4, 2018

6 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 81.987%
Details

@smashwilson smashwilson deleted the aw/pr-review-rfc branch Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment