Pull Requests

[shana]

Imported from https://github.com/github/VisualStudioInternal/issues/428

So this is the broad list of things that go into PRs. I've set checkmarks on the bare minimum PR support without which there's probably no point in shipping. Any things that we wouldn't support initially would instead jump to the website to show the information, if possible.

PR Feature - Level 1

• List Pull Requests
  • Filtering
• Create Pull Request
  • Entry form for title, description and create button
  • Assigment
  • Commit list

PR Feature - Level 2

• Create Pull Request
  • Commit detail (file changes list per commit)
  • File changes list (of the PR)
  • Diff of file change selected (from commit detail or PR changes detail), no special adornments

PR Feature - Level 3

• Pull Request detail
  • Commit list
  • Commit detail (file changes list )
  • File changes list (of the PR)
  • Conversation list
  • Diff of file change (from commit detail or PR changes list)
    • Add comment on diff view line
    • See comments on diff view line
  • Merge PR
  • Close PR

Things to figure out

• Need a way of assigning someone to a PR
• Need a merge button somewhere
• Sorting

Mockups

Pull Request List

Pull Request Creation / Detail

The pull request creation wouldn't have a Conversation tab because, well, it's a new PR, but viewing an existing PR would have it. Still debating whether to use tabs or dropdowns (for space conservation), but tabs do provide useful information.

Not seen in the mockups is the ability to see individual changes list per-commit, which is a very useful feature. Considering having the commit entry as a collapsible pane that shows a list of changed files in the same way as the PR file change list.

Pull Request Detail, alternate form with pull downs

[kevinsawicki]

/cc @github/atom

[simurai]

Still debating whether to use tabs or dropdowns (for space conservation), but tabs do provide useful information.

If space allows it, tabs would be nice. Better for orientation and quicker to access the different sections. Maybe the tabs could do without the icons? I tried it out here:

Also added:

• Create + Merge PR button
• Build check + conflict status
• A way to go back to the PR list. Could be just the back buttons at the top. Or the bread crumbs. Or a "cancel" button?

[shana]

/cc @haacked @orderedlist

[shana]

actually, /cc @github/desktop for the 👀

[shana]

Thanks for this, looks great!

For reference, this is the UI we have currently on the Team Explorer tab (pane on the right side with yellow highlighted button). It's the main "page" that shows up when you click on the Team Explorer tab, and contains several navigation items (some or ours and some aren't). The Pull Requests navigation item is ours, and it's currently set to open up the pr page on the website. The plan is to use this navitem to open the PR feature in the GitHub pane. In the future, other features (like Issues) can be added to the GitHub pane (that's the plan, anyway 😄)

A way to go back to the PR list. Could be just the back buttons at the top. Or the bread crumbs. Or a "cancel" button?

The PR icon on the toolbar on top of the pane is meant to be used as a shortcut to go back to the main PR view (which would be the list with whatever filters have been set by the user). This is a UI pattern that some bits of VS are introducing (toolbar with icons that switch between features in one pane, with back and forward buttons to navigate between areas of a particular feature), but it's not very common, so not sure how easy it is for users to pick up. Breadcrumbs could work alongside that, they're definitely easier to understand.

[simurai]

The PR icon on the toolbar on top of the pane is meant to be used as a shortcut to go back to the main PR view

I see. And yeah.. when there are more features like Issues, it will have its own icon up there.

I saw that other than the Team Explorer, it looks very close to GitHub for Windows. Like the login, clone and create repo popups. So here a version that is closer to that. Like the buttons have the circled checkmark:

Although not sure. That style works well when there is padding and white space available. But in that side panel where there is lots of stuff and that can be resized, those buttons get a bit lost.

[shana]

Although not sure. That style works well when there is padding and white
space available. But in that side panel where there is lots of stuff and
that can be resized, those buttons get a bit lost.

I really like the PR status icon on this version, fits in much better than
the green-background one!

In terms of using the circular buttons, yes, these panes have different
size constraints that can make it hard to have that much padding. Not only
is it a problem with horizontal space, but vertical space is somewhat at a
premium too - the mockups were done on a big monitor, so even though I
didn't take them with VS on full screen, I still have a lot of vertical
space there, other users can have less space.

The panes are scrollable, but action buttons should be positioned so they
don't disappear off screen if scrolling is required, so people don't get
confused about how to do things. Any extra space that buttons take is less
space for other information and more potential for scrolling.

P.S.: let me know if my mockups PSD would be useful, I can send it to you.

P.P.S.: Also, I'm doing a road trip thing, which is why my replies are
somewhat slow, sorry about that :-/

[simurai]

but vertical space is somewhat at a premium too

Yeah, also saw that vertical space can get split up with another "Properties" panel. Ok, how about smaller "Windows" style buttons, but the merge button is green:

P.S.: let me know if my mockups PSD would be useful, I can send it to you.

So far I'm fine, thanks. I started to rebuild it in Sketch. The font rendering doesn't 100% match like in Windows, but should be close.

[simurai]

Need a way of assigning someone to a PR

How about under the branch picker:

[simurai]

Commit detail (file changes list per commit)

Clicking on a commit would expand the item and show the file list (per commit). Clicking again would collapse the item.

[simurai]

Diff of file change selected (from commit detail or PR changes detail), no special adornments

I guess selecting one of the changed files would open the code diff view in VisualStudio. Something like this:

Maybe the file list items need a selected state.

---

Diff of file change (from commit detail or PR changes list)

• Add comment on diff view line
• See comments on diff view line

Does VS allow to add/see comments inline in the diff view? Or do the comments need to be shown in the GitHub panel?

[shana]

For the merge button, I would probably dock the whole thing to the bottom of the panel, so comments get their own scrolling space. I was also thinking about having it always visible no matter which view you're looking at (commits, conversation or file changes), something like the screenshot below. I would love to be able to collapse the comment box so that it doesn't occupy so much space while just being there, and only expand when the user clicks on it, that could work nicely.What do you think?

I'm thinking that the merge button might need a confirmation of some sort, it's a destructive operation and users will certainly hit the button by mistake. Maybe built in to the button itself, so it requires two clicks (that way accidental clicks will be safe but users can still merge without going through extra popups).

I guess selecting one of the changed files would open the code diff view in VisualStudio.
Maybe the file list items need a selected state.

Yup, that's the plan. Any item in a file change list should be selectable and show the corresponding diff. The mockups I posted don't show it but I switch to a darker background for selected lines.

Does VS allow to add/see comments inline in the diff view? Or do the comments need to be shown in the GitHub panel?

The plan is to have comments inline, yes. Whether it's using the built-in diff view, or whether we do our own view with diff and inline comments, I'm not sure yet, but at the end, line comments should be showing up in the diff, like we do on the website.

[haacked]

I'm thinking that the merge button might need a confirmation of some sort

Yeah, we should do it the same way github.com does it where the button turns into a confirm merge button.

[simurai]

I was also thinking about having it (merge button) always visible no matter which view you're looking at

Was thinking that too, but then I wasn't sure anymore because it might be a good thing to have the merge button only after the last comment. It makes sure you're aware of any last minute changes. Otherwise you might miss a "ohh.. just noticed "something", I'll try to fix that." I wonder if there is a specific reason why the merge button on github.com is at the end of comments and not somehwere at the top or side bar?

I would love to be able to collapse the comment box so that it doesn't occupy so much space while just being there, and only expand when the user clicks on it, that could work nicely.

Yes, that would be nice. Maybe initially just have space for 2-3 lines.