Show PR review status

[rebornix]

• Extension version: Insiders
• VSCode Version: 1.29
• OS: macOS

Now we are using Pull Requests for features and bug fixes always, so I still rely on emails and .com to know if my pr is approved and ready to be merged or not. If we can show indicators of the review status in PRs tree view, I can simply go to category Created By Me.

[StanleyGoldman]

@queerviolet and I were plotting on possible node structures we are partial to the first, but I thought we could put the others up here for discussion.

- [Checks Icon] [Review Icon] PR Title #xx by @StanleyGoldman
  - Desc
  - File 1
  - File 2
  - File N

- [Avatar] PR Title #xx by @StanleyGoldman
  - Desc
  - Reviewers
    - Ashi: +1
    - Stanley: -1
  - Checks
    - Check 1: +1
    - Check 2: +1
  - File 1
  - File 2
  - File N
  
- [Avatar] PR Title #xx by @StanleyGoldman
  - Desc
  - Reviewers: +1/-1/M
  - Checks:  +1/-1/M
  - File 1
  - File 2
  - File N

[StanleyGoldman]

Displaying two icons in the tree would not be ideal from a UX standpoint. We are trying to figure out how to merge both sets of states into one icon.

Checks states:

1. Passing
2. Failture
3. Pending

Review states:

1. Approve
2. Request Changes
3. Review Requested
4. Required

Both of those states are not all of what goes into "merge-ability".

[StanleyGoldman]

The simplest answer could very well be to display an icon for merge-ability directly.

[queerviolet]

I think there are really three basic states we want to represent:

• ✅This PR is good to merge
• ⛔️This PR can't be merged:
  • checks are failing, so fix them
  • a review is required, so assign reviewers
  • a reviewer requested changes, so make them
• ⚠️Waiting on:
  • A review from a requested reviewer
  • Checks to complete

[StanleyGoldman]

Okay, so I've been looking deeper into the initial desire and comparing with other Pull Request centric GitHub plugins do.

Both plugins:

1. Keep the users avatar in a primary position
2. Display Pull Request Checks to the right instead of a Ready to Merge indicator.
   a. The right side margin is also maintained as the panels shrink horizontally

GitHub for Visual Studio:

GitHub for Atom:

Also Visual Studio code already displays an additional checkmark (in text) to the right of a user's avatar to indicate the given pull request is the current checked out branch.

TLDR:

• For consistency, I don't think we should remove the user's avatar in favor of a different status indicator.
• I would like to add a similar status indicator to the right side of the pull request tree node if possible.
• I wonder if we could benefit from adding additional informational nodes under the pull request.

[meaghanlewis]

👋 @misolori do you have any feedback on what @StanleyGoldman described above? Specifically about adding a status indicator to the right side of the pull request tree node.

[miguelsolorio]

I think I like having the indicator in a consistent place, however we currently use a ✔️ to indicate when a PR is checked out, so that would be confusing to have two (we'd have to find a different way to differentiate that)

[srescio]

I think I like having the indicator in a consistent place, however we currently use a ✔️ to indicate when a PR is checked out, so that would be confusing to have two (we'd have to find a different way to differentiate that)

could maybe change the "checked out" indicator to bolded PR title? it would even stand out more, I think sourceTree app does that to tell you which branch you're on

[mix3d]

Why not use a branch style icon for the checked out branch, on top of the bolded title? There's nothing stopping someone from putting a ✔️ in the commit title, which would be confusing anyways.

[mix3d]

A quick mockup mixing many of the similar suggestions.

Notably missing is the PR creation date, which can be helpful when sorting through "which of these needs my attention most", though the PR # should be sufficient.

Noting that when opening one of the PRs, the immediate next row is prepended with a similar branch style icon top open the in-vscode PR editor, I would suggest either replacing with a github logo (similar to the plugin's logo), or remove the icon altogether, leaving "description" in bold and actually pulling in the first bit of the PR message body.

Could also add a row in the expanded view to explain the status icon, "Requires approval", "10/20 tests passing", etc.

Considering most people don't have dozens of PRs assigned to them at a given time, I'm wondering if the two-row layout is more useful; what more information could you add there? (like the Visual Studio example).

[digitarald]

This should also consider in-progress checks state, to make it easier to follow CI/CD results for your own PRs and the ones you are asked to review.

Related: Background updates would also be covered here: #3649 . #791 is about showing the checks status per commit.

[alexr00]

We're limited to unicode characters for the status, here's what I have so far. Circle is pending, shaded square is review required, X is CI failed.

[alexr00]

With codicons and colors instead:

[alexr00]

Tweaking some icons and the color of the checks icon:

@daviddossett, what do you think of the git-pull-request-new-changes icon for "changes requested"? Alternatively, should we have a version of the icon without the dot on it?

[mix3d]

What a journey! Love it. Thanks for all the work.

[daviddossett]

@daviddossett, what do you think of the git-pull-request-new-changes icon for "changes requested"? Alternatively, should we have a version of the icon without the dot on it?

I see we have these as of today:

..should we use request-changes? Or should that icon be updated to the "file" style pictured above if not used elsewhere?

[alexr00]

I missed the request-changes icon, that seems perfect. I'll update to use it.