Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: Review author flow #2095

Merged
merged 19 commits into from May 7, 2019

Conversation

Projects
3 participants
@vanessayuenn
Copy link
Contributor

commented Apr 24, 2019

The first draft of the review author workflow proposal is ready! This is nowhere near its final form yet, but hopefully, it will at least get the conversation started.


View rendered docs/feature-requests/006-pull-request-reviewer-flow.md

@vanessayuenn vanessayuenn requested a review from atom/github-package Apr 24, 2019

@smashwilson
Copy link
Member

left a comment

Left some initial thoughts based on the writeup 😄 Looking pretty promising 👍

I'm wondering a little about the complexity of adding a distinct additional tab for pending reviews. I like keeping them all in one place, separate from the existing reviews, but I do worry about increasing the number of navigation steps needed to reach it; GitHub tab, reviews tab, pending review tab. Can we add a "pending" section to a single reviews tab, instead? It would likely feel pretty crowded, so I'm not sure.

Show resolved Hide resolved docs/feature-requests/006-pull-request-reviewer-flow.md
Show resolved Hide resolved docs/feature-requests/006-pull-request-reviewer-flow.md Outdated
#### Responding to a comment thread
![responding to a comment thread](https://user-images.githubusercontent.com/6842965/56689748-cdd05700-66a9-11e9-90e8-266c69cbc589.png)

User can respond to a comment thread by adding a single line comment (current implementation) or starting a new review. The two buttons should only show up when the comment textbox is in focus, or is _not_ empty.

This comment has been minimized.

Copy link
@smashwilson

smashwilson Apr 29, 2019

Member

👍

We can fiddle with the "in focus or not empty" behavior once we have an implementation to play with. I'm remembering that we had to scrap the "comment editor that expands when focused" because it made the surrounding UI elements jolt around.

It also feels a little awkward to have two rows of buttons right next to one another. Not sure what else we could do there, though 🤔

Show resolved Hide resolved docs/feature-requests/006-pull-request-reviewer-flow.md Outdated
Show resolved Hide resolved docs/feature-requests/006-pull-request-reviewer-flow.md Outdated
Show resolved Hide resolved docs/feature-requests/006-pull-request-reviewer-flow.md Outdated
Show resolved Hide resolved docs/feature-requests/006-pull-request-reviewer-flow.md
@kuychaco
Copy link
Member

left a comment

Thanks for tackling this @vanessayuenn! It's super clear and thorough

Left a few questions to ponder, all pretty minor



### "All Reviews" tab
This tab shows all review summaries and review comments, including the ones that are part of a _pending review_ that has not been submitted yet.

This comment has been minimized.

Copy link
@kuychaco

kuychaco Apr 29, 2019

Member

Hmm... since the pending review will be in the "All Reviews" tab, is it necessary to have a separate "Pending Review" tab?

Quite possibly. I haven't thought too deeply about it. Just wanted to throw out the question.

This comment has been minimized.

Copy link
@vanessayuenn

vanessayuenn May 1, 2019

Author Contributor

My major concerns with keeping both published comments and pending comments in the same space are that 1) it would get too crowded, and 2) there would be no obvious hierarchy in the information presented.

I would argue that when a user is in the middle of reviewing a PR (i.e. have a review pending), the most relevant information to them would be their pending comments, as well as the summary textbox that they need to fill out before publishing a review. I couldn't figure out a way to fit all this along with other published comments, while preserving the hierarchy of importance.


#### Pending comments

![pending comment](https://user-images.githubusercontent.com/6842965/56692893-2ce59a00-66b1-11e9-81cc-bc7956bc8bec.png)

This comment has been minimized.

Copy link
@kuychaco

kuychaco Apr 29, 2019

Member

Oooh interesting. I kinda like this placement, and I like the arrow that indicates it'll take you to somewhere else if you click it.

Minor UI question...

I think our current implementation has the badge on the top of the comment next to the author name. Should we consider keeping it up there? Or do we want to make it more pronounced like it is here?

This comment has been minimized.

Copy link
@vanessayuenn

vanessayuenn May 1, 2019

Author Contributor

Hrmm yea honestly I am 50/50 on this one. I am open to fiddle with it once we are at implementation stage, and then decide which treatment has better UX.

This comment has been minimized.

Copy link
@smashwilson

smashwilson May 3, 2019

Member

Yeah - the existing "pending" badge is at the top. But it also has no click action. I think being actionable would justify moving it below though. 🤔


![pending review summary](https://user-images.githubusercontent.com/6842965/56699584-23196200-66c4-11e9-94a4-193c9d662bb3.png)

The summary section of the Pending Review tab is sticky (although still collapsible), so it stays within view regardless of how long the list of comments below it is. The icon on the left indicates the type of review, which can be selected in the dropdown underneath the text box. The button to submit review will be disabled a review type has not been chosen from the dropdown menu.

This comment has been minimized.

Copy link
@kuychaco

kuychaco Apr 29, 2019

Member

Does this imply we won't have a default? Looks like dotcom has "Comment" as default (at least for me)

This comment has been minimized.

Copy link
@vanessayuenn

vanessayuenn May 1, 2019

Author Contributor

Hrmm.. I can see the value in both having "Comment" as a preseleted default, and making users explicitly choose the type of review they're making. I think I am indifferent? Do you have a preference for one?

This comment has been minimized.

Copy link
@smashwilson

smashwilson May 3, 2019

Member

Preselecting it would reduce the interactions required to submit a comment review, but increase the number of users who submit one by accident or without thinking. I'm kind of indifferent too, but I think slightly on the side of not having a default and making the user make a conscious choice... ?

Show resolved Hide resolved docs/feature-requests/006-pull-request-reviewer-flow.md Outdated
Update docs/feature-requests/006-pull-request-reviewer-flow.md
Co-Authored-By: vanessayuenn <6842965+vanessayuenn@users.noreply.github.com>
@vanessayuenn

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@smashwilson:

I'm wondering a little about the complexity of adding a distinct additional tab for pending reviews.

That's a very valid point. Another thing I just thought of is that while a user can only have one pending review per PR, it is entirely possible for a user to have a pending review for multiple PRs, which would then result in 2 review tabs per PR -- that can get confusing really fast.

I started exploring the option of having "sub-tabs" within a review tab 👇. That way we can keep all reviews (pending and published) for a PR in one place, with a simpler navigation model.

image

@codecov

This comment has been minimized.

Copy link

commented May 1, 2019

Codecov Report

Merging #2095 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2095      +/-   ##
==========================================
+ Coverage   92.55%   92.55%   +<.01%     
==========================================
  Files         207      207              
  Lines       12021    12016       -5     
  Branches     1746     1745       -1     
==========================================
- Hits        11126    11122       -4     
+ Misses        895      894       -1
Impacted Files Coverage Δ
lib/views/reviews-view.js 82.71% <0%> (-0.52%) ⬇️
lib/atom/gutter.js 92.3% <0%> (+2.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ef8aac...b6c4789. Read the comment docs.

vanessayuenn added some commits May 1, 2019

@vanessayuenn vanessayuenn requested review from smashwilson and kuychaco May 1, 2019

@vanessayuenn vanessayuenn changed the title [WIP] Review author flow Review author flow May 1, 2019

@smashwilson
Copy link
Member

left a comment

👍 I think we've got enough to get started 😄

Show resolved Hide resolved docs/feature-requests/006-pull-request-reviewer-flow.md

#### Pending comments

![pending comment](https://user-images.githubusercontent.com/6842965/56692893-2ce59a00-66b1-11e9-81cc-bc7956bc8bec.png)

This comment has been minimized.

Copy link
@smashwilson

smashwilson May 3, 2019

Member

Yeah - the existing "pending" badge is at the top. But it also has no click action. I think being actionable would justify moving it below though. 🤔


![pending review summary](https://user-images.githubusercontent.com/6842965/56699584-23196200-66c4-11e9-94a4-193c9d662bb3.png)

The summary section of the Pending Review tab is sticky (although still collapsible), so it stays within view regardless of how long the list of comments below it is. The icon on the left indicates the type of review, which can be selected in the dropdown underneath the text box. The button to submit review will be disabled a review type has not been chosen from the dropdown menu.

This comment has been minimized.

Copy link
@smashwilson

smashwilson May 3, 2019

Member

Preselecting it would reduce the interactions required to submit a comment review, but increase the number of users who submit one by accident or without thinking. I'm kind of indifferent too, but I think slightly on the side of not having a default and making the user make a conscious choice... ?

@vanessayuenn vanessayuenn changed the title Review author flow Feature Request: Review author flow May 7, 2019

@vanessayuenn vanessayuenn merged commit e0b5fb4 into master May 7, 2019

14 checks passed

atom.github Build #20190501.35 succeeded
Details
atom.github (Lint) Lint succeeded
Details
atom.github (Linux beta) Linux beta succeeded
Details
atom.github (Linux dev) Linux dev succeeded
Details
atom.github (Linux stable) Linux stable succeeded
Details
atom.github (MacOS beta) MacOS beta succeeded
Details
atom.github (MacOS dev) MacOS dev succeeded
Details
atom.github (MacOS stable) MacOS stable succeeded
Details
atom.github (Snapshot) Snapshot succeeded
Details
atom.github (Windows beta) Windows beta succeeded
Details
atom.github (Windows dev) Windows dev succeeded
Details
atom.github (Windows stable) Windows stable succeeded
Details
codecov/patch Coverage not affected when comparing 4ef8aac...b6c4789
Details
codecov/project 92.55% (+<.01%) compared to 4ef8aac
Details

Sprint : 4 April 2019 - 8 May 2019 : v0.29.0 automation moved this from In progress to Merged May 7, 2019

@vanessayuenn vanessayuenn deleted the vy/meta-rfc-pr-reviewer-flow branch May 7, 2019

@smashwilson smashwilson referenced this pull request May 8, 2019

Closed

v0.29.0-0 QA Review #2129

11 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.