Visualstudio: Pull requests: details view exploration

[donokuda]

Exploring another layout treatment for the pull request details view. The layout is inspired by some of @simurai's work for Atom ✨:

The top part of the details view would have a summary of the pull request. At a quick glance, you should be able to understand the following:

• What is the current state of the PR?
• How large is the branch?
• Is this branch stable?
• Can this branch be merged or are there conflicts that need to be resolved first?

If there is something with the branch that needs to be addressed, then we should be able to guide the developer to solving it. For example, if there are builds failing, we could link to a list of build failures for that pull request on GitHub.com. If everything looks good, then we could merge the branch from this view (if that's something we want to support.)

I also thought about how we could support opening and closing a pull request in the UI. One idea was to have the open/close badge be a dropdown. Another idea is to have the "close" button next to the comment button (closer to how we do it on GitHub.com)

Below the branch summary will show more details for the pull request. From left to right are sketches for details, commits, files changed, and inline (code) comments.

You could navigate between the different views (details, code comments, commits, files changed) via a dropdown.

This navigation treatment could have the benefit of allowing us to add more views in the future. For example, if we were to add a "builds" view, it would appear as another choice in the dropdown.

Feedback welcome 🙇

(/cc @shana @simurai)

[simurai]

One idea was to have the open/close badge be a dropdown.

Heh.. that's a great way to optimize the available space. :sunglasses:

Another idea is to have the "close" button next to the comment button (closer to how we do it on GitHub.com)

That's a good question: How easy should it be to merge/close PRs? I kinda remember talking about this here. Also see @shana's next comment about .com being a different context.

Maybe it could be like in this issue, but make sure you wouldn't miss any new comments. Like for the first time when opening a PR, the scroll position would be at the top, where the details are. But then when opening the next time, the scroll position would start with the new unread comments. Like that blue (+) on .com: Pick up where you left off. Or maybe better, have some indicator (blue dot with unread count) that there are new comments that you can click and scroll to them. Then you less likely miss those "wait wait with merging" comments.

This navigation treatment (dropdown) could have the benefit of allowing us to add more views in the future.

It adds an extra click (or hover), but yeah, it's a lot more flexible and no worries about having enough space. Which could be an issue with tabs. There might be ways to add shortcuts, like with cmd + tab you could cycle through them. Or maybe add a "next" button ( > arrow to the right). Multiple clicks could still be faster than moving your mouse to the right list item.

[donokuda]

@simurai Thanks for the feedback! 🙇

How easy should it be to merge/close PRs? I kinda remember talking about this here. Also see @shana's next comment about .com being a different context.

Totally agree on what was discussed previously about keeping the merge button close to the bottom instead, specifically:

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

and

any button that acts on something specific needs to be close to that specific thing, and the central column on the website is where actions for the content go - so the merge button has to be in the center along with its comment box, and can't be on the sidebar. It could be above the comments, but then the reason you mentioned probably comes into play - you don't want people merging without reading all the comments.

I reworked the layout back to having the merge/close pull request actions at the bottom. I also explored your idea of having the comment box collapsed at first and expanded when it's in focus.

When the comment box isn't in focus, then the merge and close buttons are available. If the comment box is in focus, then those actions are replaced with your standard Cancel and Comment buttons:

Maybe it could be like in this issue, but make sure you wouldn't miss any new comments. Like for the first time when opening a PR, the scroll position would be at the top, where the details are. But then when opening the next time, the scroll position would start with the new unread comments. Like that blue (+) on .com: Pick up where you left off.

💯 to making sure you haven't skipped over any new information before deciding to either merge or close a pull request.

I like the idea of jumping down to unread comments when you're revisiting a pull request. We could call out new comments with a separator similar to how we do on dotcom:

Or maybe better, have some indicator (blue dot with unread count) that there are new comments that you can click and scroll to them. Then you less likely miss those "wait wait with merging" comments.

We could call out that there are new comments or changes near the top of the view. Here's another variation that combines the merge and build status to one line (to save space since two lines aren't needed.) I also tried a version inspired by Visual Studio's infobar UI pattern.

Or maybe add a "next" button ( > arrow to the right). Multiple clicks could still be faster than moving your mouse to the right list item.

I sketched out a few variations and narrowed on a couple interesting approaches. I do think it is quicker to move around the different views, but I'm worried it might not be clear how many and what views are available to you without having to discover them first.

[simurai]

I'm worried it might not be clear how many and what views are available to you without having to discover them first.

Yeah, true. The "next/prev" could be in addition to the dropdown and not the only way. Or more hidden and for Power Users™. Like alt clicking the name could jump to the next one.

[shana]

We could call out that there are new comments or changes near the top of the view. Here's another variation that combines the merge and build status to one line (to save space since two lines aren't needed.) I also tried a version inspired by Visual Studio's infobar UI pattern.

Oh, I do like this approach, it feels very natural to the UI!

[shana]

One idea was to have the open/close badge be a dropdown.

We would probably need a confirmation dialog/popup/message of some sort to make sure the user knows what's going to happen, since it's probably very easy to change the state by mistake (for instance, if the dropdown has focus and the user uses the mouse scroll wheel).

[shana]

This navigation treatment could have the benefit of allowing us to add more views in the future. For example, if we were to add a "builds" view, it would appear as another choice in the dropdown.

Excellent point. This will give us the flexibility to slowly add features without having to radically change the UI. I like the option of prev/next buttons to quickly cycle through the views for power users in addition to a dropdown. This is awesome!

[shana]

So between having a dropdown for the Open/Closed state of the PR, and having a Close button here, I'm torn... A dropdown implies that you can switch between states, and while that is certainly true for PRs, is it something that happens often?
OTOH, with only the button, how do you reopen a PR that's been closed (intentionally or not?). What does a PR in the closed state look like? Replace the merge and close buttons with a "Reopen" button?

[donokuda]

So between having a dropdown for the Open/Closed state of the PR, and having a Close button here, I'm torn... A dropdown implies that you can switch between states, and while that is certainly true for PRs, is it something that happens often?

OTOH, with only the button, how do you reopen a PR that's been closed (intentionally or not?). What does a PR in the closed state look like? Replace the merge and close buttons with a "Reopen" button?

Yup! It makes more sense to have the reopen as a button for the same reason the close button should appear after the conversation: reopening is a decision that's made after reading the conversation.

Here are a couple of explorations for the "closed" case:

The left example keeps the same layout as an opened pull request but replaces the action buttons with a single "reopen" pull request. The amount of space taken seems unnecessary given that there's only one action to take.

The right example takes up only one row. Attention is less drawn to this area, which might be nice, although I worry about the amount of horizontal spacing we realistically have for information.

I'm also thinking that if the branch is merged, then nothing will appear under the comment field:

What do you all think?

[shana]

I think this looks great! +1 on only doing one row with the Reopen button on the right side. Do we need to have the whole "Closed with unmerged commits" phrase in there? I'm thinking that if there's no unmerged commits, then there's really no need to have the Reopen button at all, since the merge will be a no-op.

@Haacked thoughts?

[donokuda]

Do we need to have the whole "Closed with unmerged commits" phrase in there? I'm thinking that if there's no unmerged commits, then there's really no need to have the Reopen button at all, since the merge will be a no-op.

Does having no unmerged commits imply that the branch is merged? In that case, we'll update the view to reflect that (such as updating the PR status badge to "Merge" and not showing anything underneath the comment field.)

[haacked]

Does having no unmerged commits imply that the branch is merged?

Yeah, pretty much. If you happen to merge a PR branch into master, .com automatically closes the PR.

In that case, we'll update the view to reflect that (such as updating the PR status badge to "Merge" and not showing anything underneath the comment field.)

This seems right. Although it should be "Merged" and not "Merge" for the status.

Do we need to have the whole "Closed with unmerged commits" phrase in there?

I think the phrase is useful. Users might not necessarily pick up on the subtle distinction between closed and merged. The "unmerged commits" lets them know that this was closed with potentially pending work on the PR.

On the other hand, I don't feel too strongly about it. If you happened to see a list of PRs where some of them say "Merged" and others say "Closed" I think you'd pick up on the distinction real fast. :smile:

[donokuda]

Few quick explorations of what the layout could look like for reopening a pull request. The last one is looking the most promising:

I'll admit that I was using the "Closed with unmerged commit" text as a layout crutch so I can nicely center the reopen button at the bottom. 😄 However, I'm not committed to the text either way (especially if the text isn't very valuable and there's an alternative that works well.)

[shana]

I have to say, I'm really loving all these mockups!

I'll admit that I was using the "Closed with unmerged commit" text as a layout crutch so I can nicely center the reopen button at the bottom.

Oh well, then by all means let's use it! It does look nice 😄
I think the minimum pane size has enough width to always have the text, so it's fine.

Have you had any thoughts on how we're going to include reactions in this UI? Not that we have the API for them yet, but we will by the time we get to it 😉

[donokuda]

Have you had any thoughts on how we're going to include reactions in this UI? Not that we have the API for them yet, but we will by the time we get to it 😉

ooo good call! I'll sketch some ideas out along with multiple assignees and open issues to discuss them.

[donokuda]

Done via #730 although the final product doesn't look too much like these explorations from June! 😉