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

Update RFC to reflect editor-first design #1712

Merged
merged 3 commits into from Oct 4, 2018

Conversation

Projects
None yet
4 participants
@kuychaco
Member

kuychaco commented Oct 3, 2018

Many thanks to @simurai for helping me flesh out the new design and making pretty mockups

This PR contains a (hopefully) comprehensive description of the editor-first code review experience. Some mockups are still under constructions, but I think you can get the gist of things based on what we have so far.

@smashwilson, @annthurium, @vanessayuenn, @sguthals -- We welcome thoughts, concerns, questions, etc.

I believe this addresses a majority of the concerns and open design questions we discussed in #1706. Namely,

  • PR tile in GitHub tab getting out of sync with active pane item and leading to confusion
  • Redundant information across "Changes" tab, "Reviews" tab, and text editor with inline comments

⚡️:octocat: (rendered) :octocat:⚡️

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

Thank you for writing all of this up 😍 I am:

  • 👍 on the diff-focused PullRequestPaneItem, pretty much exactly as described
  • 👍 on simplifying the pull request tiles in the GitHub tab
  • 👍 on specifying diff editability. I still have a bunch of questions about how it would work, but no showstoppers and I'm totally willing to defer them to implementation time
  • 👎 on removing the in-editor decorations - I still think these are important and I really don't want to lose them

As always I've left a ton of comments, but the only thing I'd like to resolve before merge is that last one ☝️ . I'd also like to make sure @sguthals, @annthurium, and @vanessayuenn are on-board with the change in direction, since it's a pretty big change from the document we talked about yesterday!

Show resolved Hide resolved docs/rfcs/XXX-pull-request-review.md Outdated
Show resolved Hide resolved docs/rfcs/XXX-pull-request-review.md
Show resolved Hide resolved docs/rfcs/XXX-pull-request-review.md
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.

This comment has been minimized.

@smashwilson

smashwilson Oct 3, 2018

Member

Here's how I'm interpreting some of the implications when you say "diffs are editable":

  • The multi-file diff that we're displaying is actually generated between the PR's merge base and the local working copy for the checked-out PR. That is: it includes changes from commits on the local PR branch that have not been pushed yet and changes that have not been committed yet.
    • Note that this is different from the diff that's shown on dotcom! I'm pretty sure this is the right thing to do (that extra data is part of the value we can add). But it may be a source of confusion. I've encountered many users struggling to get the diff on dotcom to be what they expect, and I can see them trying to use this as a guide to accomplish that. Maybe there's a way we can communicate what's going on more clearly when you have? I'm not sure.
    • What diff should we show for non-checked-out PRs? If a non-checked-out PR has a local tracking branch that has unpushed commits, should changes from those be shown as well? How can we communicate that?
  • Editing within different regions of the PR diff is handled as follows:
    • Modifying an unchanged row edits the underlying file directly. Note that it will immediately change into a removed and addition row, which git may group with surrounding addition/removal regions depending on the whims of the diffing algorithm. We'll have to make sure this transition isn't too jarring; maybe preserving the cursor position and scroll will be enough?
    • Modifying an added row also edits the underlying file directly. (This is the easy case 😄)
    • Modifying a removed row could either: (a) be disallowed by the editor; or (b) immediately introduce the edited row as an addition with the change applied. I'm curious which you had in mind!
    • A nonewline row may not be edited, if present. (Maybe we could have a context menu option to add the newline... ?)
  • Does editing the diff interact with the file modification system? Meaning: when you edit a diff, does the pane get marked "dirty" and require a "save" for the changes to propagate to the underlying file, or are the changes applied directly? What about other editors open on the same file?

Also: it would feel really weird to me to be able to edit PR diffs before you could edit local diffs in a regular FilePatchEditor. If so, we should make sure we use identical semantics for diff edits in both contexts. This is different from how I was picturing FilePatch editing! I'd been thinking of it like choosing "e" for "edit" in git add -p -- a chance to edit the diff hunk before it's applied to the index, independently of the underlying file.

screen shot 2018-10-03 at 8 28 21 am

My go-to motivating use case for this is removing .only() from Mocha test cases. But - I think editing through to the underlying file is perfectly reasonable and useful here, too... so let's do that instead 😀


To be clear: I don't think we need to have answers for everything here to ship the RFC (and certainly not to merge this PR). But I suspect you have some intuition around some of these points already, and I'm mostly curious to hear what it is!

This comment has been minimized.

@annthurium

annthurium Oct 3, 2018

Contributor

@kuychaco do you think it's going to cause user confusion when diffs aren't editable, because the local branch history has diverged? How will we inform users about that?

This comment has been minimized.

@kuychaco

kuychaco Oct 3, 2018

Member

@smashwilson and @annthurium these are great questions! And I'm happy to address them. Though I think I'm going to defer that until after getting the big picture of this RFC sorted out. Then I can open another RFC PR for this feature and we get dig into all the fun nitty gritty details :)

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.

This comment has been minimized.

@smashwilson

smashwilson Oct 3, 2018

Member

This doesn't feel like "sorting" to me at all. "Sorting" implies that we'd only be re-ordering things that are already visible, but implementing this will involve adding and removing UI elements, rendering some files twice, showing and hiding diff lines, and so on.

I'm 👍 on the functionality! This is a really cool take on the problem. I'd just rather find a better verb for the UI. Maybe something like "show"?

This comment has been minimized.

@smashwilson

smashwilson Oct 3, 2018

Member

Also: how were you picturing the transitions between states working? Can we preserve your scroll position or focus somehow, so you can not lose your place within a review... ?

This comment has been minimized.

@annthurium

annthurium Oct 3, 2018

Contributor

+1 - calling this "sorting" to me implied that we'd be sorting diffs alphabetically by filename. Since that's not what's happening, we need a different word. Possibly a dropdown would be better here than radio buttons, since dropdowns are more traditionally used for "select a different view of this data."

I'm also really confused by the "sorting files by type" functionality in the image.

This comment has been minimized.

@kuychaco

kuychaco Oct 3, 2018

Member

Totally onboard with these suggestions. Let's get @simurai's thoughts.

I'm also really confused by the "sorting files by type" functionality in the image.

Yeah, sounds like there's more UI refinement needed here.

xxx-pull-request-review_md_preview_ ___github_github

I think you're referring to the "JS CSS JSON" part of the image. This would be useful if you're a designer and mostly interested in checking out the CSS files and you want to filter the diffs to show only CSS files. We can probably find a way of making this more clear. I expect these details will change anyhow as we're implementing. And this particular thing seems like a lower priority feature anyhow, in my opinion.

This comment has been minimized.

@simurai

simurai Oct 4, 2018

Member

I'm also really confused by the "sorting files by type" functionality in the image.

The "sorting files by type" is similar to search by language on .com. So you could narrow down the changes to certain languages. To make all these options less overwhelming, it could be hidden behind a search input. Like language:Less. And it's definitely just a "nice to have", but might be handy for bigger PRs.

Here a new mockup that is a bit more cleaned up. The radios are replaced by a "segmented" control.

screen shot 2018-10-04 at 11 07 57 am

I'd just rather find a better verb for the UI. Maybe something like "show"?

An alternative might be "Group by"? Group all changes in a different way.

Also: how were you picturing the transitions between states working? Can we preserve your scroll position or focus somehow, so you can not lose your place within a review... ?

That's a good point. I assume that's not really possible, at least not always. Because the diff you're currently looking at, might not be part of any review. Maybe we should use tabs? Then the scroll position is not expected to be the same. lol And we came full circle. 🤣

OK, I think this RFC is mostly trying to say:

  1. Let's move away from .com's UI. Why: If it looks the same/similar, people also expect the same functionality. Which would be a huge effort.
  2. Let's make more use of unique features/workflows that an editor can offer, e.g editable diffs. Why: To have more reasons to actually use an editor for code review.

If we compare the mockups from both PRs, they are not that different. The second is more about "making the changes the main thing of a PR" vs. the "Conversation".

screen shot 2018-10-04 at 12 09 45 pm

* 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.
<img width="722" alt="slack_-_github" src="https://user-images.githubusercontent.com/7910250/46391893-fbb16800-c693-11e8-88e7-ffe73448f8a8.png">

This comment has been minimized.

@smashwilson

smashwilson Oct 3, 2018

Member

Neat

One of my primary uses for the pull request panes today is watching build statuses - I open the pane and move it to a dock while I'm working on something else in the center. To support this the panel here should be toggleable rather than appear-on-hover like a tooltip. I think that's what we were thinking anyway but just making sure 😉

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

This use case is good to note. I'm 👍 for toggleable

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.

This comment has been minimized.

@smashwilson

smashwilson Oct 3, 2018

Member

Does this include a way to add new comments? Or do you need to expand it to its own pane to do that?

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

Yes, I'm imagining that it would include a way to add new comments

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.

This comment has been minimized.

@smashwilson

smashwilson Oct 3, 2018

Member

Okay, I think I feel about as strongly about this one as you do about the diff being editable, @kuychaco. One of the most important things I want from in-editor PR review as a PR author is not having to manually hunt for where review comments were made in my working copy.

Editing diffs directly is ideal for applying cosmetic and local changes... but there are plenty of comments that can't be addressed locally: renaming a variable throughout a file or a codebase; importing a different utility function to use; refactoring a method from the commonalities between two different, distant methods. When I can, I definitely do want to fix what I can right there in the diff without having to break my review-reading flow. But when I hit a comment that requires a more involved resolution I really want to be able to carry that context through to my normal, comfortable, uncramped editing environment... and then go back to the review-reading once I've addressed it.

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

@smashwilson totally totally. I was trying to manage scope and do some prioritizing, since this RFC already feels big. That said, I'm totally happy to keep in-editor PR comments in this RFC and give it it's fair chance at prioritization.

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?

This comment has been minimized.

@smashwilson

smashwilson Oct 3, 2018

Member

Hah! Yup yup. I'd really rather not use additional colors here though, I feel like that would get confusing really quickly, especially if you ever change themes.

This comment has been minimized.

@annthurium

annthurium Oct 3, 2018

Contributor

Yeah. Colors are hard from an accessibility standpoint, and also because Atom's themes and colors are inconsistent.

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

Noted.

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

This comment has been minimized.

@smashwilson

smashwilson Oct 3, 2018

Member

👍 Yeah, I think I like the simpler and more consistent pull request tiles.

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.

This comment has been minimized.

@annthurium

annthurium Oct 3, 2018

Contributor

@kuychaco do you think it's going to cause user confusion when diffs aren't editable, because the local branch history has diverged? How will we inform users about that?

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.

This comment has been minimized.

@annthurium

annthurium Oct 3, 2018

Contributor

+1 - calling this "sorting" to me implied that we'd be sorting diffs alphabetically by filename. Since that's not what's happening, we need a different word. Possibly a dropdown would be better here than radio buttons, since dropdowns are more traditionally used for "select a different view of this data."

I'm also really confused by the "sorting files by type" functionality in the image.

![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.

This comment has been minimized.

@annthurium

annthurium Oct 3, 2018

Contributor

I'm still not sold on this new direction. This is a big change to the direction we've been moving in for months, and you haven't presented compelling evidence for how this would benefit our users.

Can you add some user stories here about what you can do with this approach, that would be different from the functionality offered by Ash's original proposal?

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?

This comment has been minimized.

@annthurium

annthurium Oct 3, 2018

Contributor

Yeah. Colors are hard from an accessibility standpoint, and also because Atom's themes and colors are inconsistent.

@@ -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?

This comment has been minimized.

@annthurium

annthurium Oct 3, 2018

Contributor

How can we notify users when new information, including reviews, is available is listed below as out of scope for this RFC. This question feels related to that question, and it's my opinion that it's also out of scope.

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>
@kuychaco

Thanks for all the comments @smashwilson and @annthurium! I did my best to incorporate them.

<img width="731" alt="slack_-_github" src="https://user-images.githubusercontent.com/7910250/46392672-03264080-c697-11e8-8fe4-04605a4d5b13.png">
Clicking the "Review Changes" button reveals a UI much like dotcom's

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

Good question. That's still TBD. @simurai is gonna play with some mockups to see what would work best for us.

* 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.
<img width="722" alt="slack_-_github" src="https://user-images.githubusercontent.com/7910250/46391893-fbb16800-c693-11e8-88e7-ffe73448f8a8.png">

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

This use case is good to note. I'm 👍 for toggleable

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.

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

Yes, I'm imagining that it would include a way to add new comments

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.

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

@smashwilson totally totally. I was trying to manage scope and do some prioritizing, since this RFC already feels big. That said, I'm totally happy to keep in-editor PR comments in this RFC and give it it's fair chance at prioritization.

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?

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

Noted.

@smashwilson smashwilson merged commit 58863e1 into aw/pr-review-rfc Oct 4, 2018

3 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

@smashwilson smashwilson deleted the ku/pr-review-rfc-editor-focus branch Oct 4, 2018

@smashwilson

This comment has been minimized.

Member

smashwilson commented Oct 4, 2018

Continuing in #1706 Thanks for this super detailed exploration @kuychaco and @simurai!

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