/VisualStudio

Show `Pull requests` button on status bar when there's no active PR branch #1593

Merged
merged 11 commits into from Apr 17, 2018
+129 −39

Conversation

Reviewers

@grokys grokys

@donokuda donokuda

Assignees
No one assigned
Labels
Projects
  
Done in 2.4.4
Milestone
No milestone
5 participants
@jcansdale @sguthals @meaghanlewis @donokuda @grokys
@jcansdale
Contributor

jcansdale commented Apr 10, 2018 •
edited
Edited 19 times
  • @jcansdale jcansdale edited Apr 17, 2018
  • @jcansdale jcansdale edited Apr 17, 2018
  • @jcansdale jcansdale edited Apr 17, 2018
  • @jcansdale jcansdale edited Apr 17, 2018
  • @jcansdale jcansdale edited Apr 17, 2018
  • @jcansdale jcansdale edited Apr 12, 2018
  • @jcansdale jcansdale edited Apr 12, 2018
  • @jcansdale jcansdale edited Apr 12, 2018
  • @jcansdale jcansdale edited Apr 12, 2018
  • @jcansdale jcansdale edited Apr 12, 2018
  • @jcansdale jcansdale edited Apr 12, 2018
  • @jcansdale jcansdale edited Apr 12, 2018
  • @jcansdale jcansdale edited Apr 11, 2018
  • @jcansdale jcansdale edited Apr 11, 2018
  • @jcansdale jcansdale edited Apr 11, 2018
  • @jcansdale jcansdale edited Apr 11, 2018
  • @jcansdale jcansdale edited Apr 11, 2018
  • @jcansdale jcansdale edited Apr 11, 2018
  • @jcansdale jcansdale edited Apr 11, 2018

This PR will show a Pull requests button on the status bar when in the context of a GitHub repo but not on a PR branch.

What it does

  • Make the OpenPullRequests button visible when in the context of a repo with a remote
  • Make the ShowCurrentPullRequest button visible when on an PR branch

2018-04-12_21-37-33

This means that if we're on a PR branch that hasn't yet been opened in the PR detail view (issue #1591), the OpenPullRequests button will still be visible and encourage the user to open the PR by hand. This is also useful when the user simply want to change to a different PR.

The Pull requests button has same capitalization as on github.com (slightly to screen-cast above)

image

image

Todo

  • Decide what to show when the user is on a GitHub repo, but not on a PR branch (or we don't know)

    • The icon + Pull requests would be consistent with .com

  • Be more selective about when the OpenPullRequests button appears

    • Show Pull requests when repo has a remote

At the moment it will appear whenever there is a Git repo with a remote. This means it will also appear on a TFS Git repo. While not the end of the world (clicking on it says This repository is not on GitHub), it would be nice if we could be a little more selective.

Do we currently have any code that looks for GitHub-ish URLs?

TFS Git remotes seem to look like this (which would be easy to filter out):
https://user.visualstudio.com/defaultcollection/_git/RepoName

Reviewers

  • Sanity check my use of Rx in PullRequestStatusBarManager.StartShowingStatus

  • I've noticed the tooltip for the OpenPullRequests icon doesn't always appear. Any idea what's going on here?

  • The tooltip for our OpenPullRequests command on the toolbar is simply Pull Requests. In this context I'm using Open or Create Pull Requests. What do you prefer out of these:
    Pull Requests, Open or Create Pull Requests, Open or Create Pull Request - or something else?

  • After splitting the button in two, it the spacing okay? @donokuda

Rejected

Showing OpenPullRequests and ShowCurrentPullRequest as separate buttons. Not consistent with other Team Explorer status bar buttons/menus.

2018-04-11_11-01-01

Future PRs

  • It would be nice if we could point people towards adding a repo to GitHub when the repo doesn't have a remote.

Mitigation for #1591

jcansdale added some commits Apr 10, 2018

@jcansdale
Split PR status into Open PRs / Show PR buttons
c3cff01
@jcansdale
Only show PR status bar if repo has remote
Loading status checks…
211719b
@jcansdale
Merge branch 'master' into enhancement/1591-status-bar-open-PRs
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
dec0a62
@sguthals

This comment has been minimized.

Show comment Hide comment
@sguthals

sguthals Apr 12, 2018

Contributor

Just some overall comments from our conversations today:

High Level Problem

There are three issues that we currently have:
Giving users a way to...

  1. open the GitHub pane
  2. get back to the list of PRs
  3. open a new PR
    That isn't going through levels of menus or through the Team Explorer Pane when the GitHub pane isn't already open.

Issues with this solution

Though the solution proposed here might resolve the friction of the above three problems, it introduces new friction in terms of design continuity and user experience expectations. Mainly:

  1. The task bar sets the tone for icon+text and that should not be broken.
  2. A user might:
    a. Think that the highlighting is broken because the icon or text isn't being highlighted when the other is hovered over
    b. Always click on the text part and get used to that taking them to the details, but then accidentally click on the icon part one day and be taken to a completely different view

Other Potential Solutions to the High Level Problem

I feel there are two things that need to be addressed:

  1. Specifically the PR accessibility (list vs detail vs open)
  2. The GitHub pane accessibility

Focused on solving the PR accessibility issue

If we are focused on only solving the first, I could imagine a few potential solutions:

  1. Add an icon+text that is the PR icon and the number of PRs for this repo, which would take the user to the list of PRs.
  2. Add a GitHub icon+text that would take the user to the GitHub pane, which right now only has the list of PRs anyway
  3. Add a menu on the current icon+text that comes up and has three options:
    a. Open new PR
    b. Open PR List
    c. Open current PR details
    We do not have to also list the PRs in this menu. But it would better follow the UI with the branches menu that Team Explorer has already introduced:

screen shot 2018-04-11 at 7 30 11 pm

I prefer (3), however I'm wondering what we should do if a user isn't on a PR. What should the text say?

And I'm sure many other ideas.

Focused on the GitHub pane discoverability, workflow, accessibility

I do think, with Forking, Issues, and PR Reviews we have a bigger issue that needs addressing. That is - how are we going to help users be able to discover all of our features when they need to be discovering them.
Contributor

sguthals commented Apr 12, 2018 •
edited
Edited 1 time
  • @sguthals sguthals edited Apr 12, 2018

Just some overall comments from our conversations today:

High Level Problem

There are three issues that we currently have:
Giving users a way to...

  1. open the GitHub pane
  2. get back to the list of PRs
  3. open a new PR
    That isn't going through levels of menus or through the Team Explorer Pane when the GitHub pane isn't already open.

Issues with this solution

Though the solution proposed here might resolve the friction of the above three problems, it introduces new friction in terms of design continuity and user experience expectations. Mainly:

  1. The task bar sets the tone for icon+text and that should not be broken.
  2. A user might:
    a. Think that the highlighting is broken because the icon or text isn't being highlighted when the other is hovered over
    b. Always click on the text part and get used to that taking them to the details, but then accidentally click on the icon part one day and be taken to a completely different view

Other Potential Solutions to the High Level Problem

I feel there are two things that need to be addressed:

  1. Specifically the PR accessibility (list vs detail vs open)
  2. The GitHub pane accessibility

Focused on solving the PR accessibility issue

If we are focused on only solving the first, I could imagine a few potential solutions:

  1. Add an icon+text that is the PR icon and the number of PRs for this repo, which would take the user to the list of PRs.
  2. Add a GitHub icon+text that would take the user to the GitHub pane, which right now only has the list of PRs anyway
  3. Add a menu on the current icon+text that comes up and has three options:
    a. Open new PR
    b. Open PR List
    c. Open current PR details
    We do not have to also list the PRs in this menu. But it would better follow the UI with the branches menu that Team Explorer has already introduced:

screen shot 2018-04-11 at 7 30 11 pm

I prefer (3), however I'm wondering what we should do if a user isn't on a PR. What should the text say?

And I'm sure many other ideas.

Focused on the GitHub pane discoverability, workflow, accessibility

I do think, with Forking, Issues, and PR Reviews we have a bigger issue that needs addressing. That is - how are we going to help users be able to discover all of our features when they need to be discovering them.

@jcansdale jcansdale changed the title from Show "Open PRs" button on status bar when there's no active PR branch to Show `Pull requests` button on status bar when there's no active PR branch Apr 12, 2018

@jcansdale jcansdale requested a review from grokys Apr 13, 2018

@jcansdale jcansdale added the Ready for Review label Apr 13, 2018

@jcansdale jcansdale requested a review from donokuda Apr 13, 2018

@donokuda

donokuda reviewed Apr 13, 2018
View changes

This is a reasonable stop-gap until we take a look at the extension as a whole to improve discoverability. 👍

Thanks for kicking off this initiative, @jcansdale!

@meaghanlewis meaghanlewis added this to In progress in 2.4.4 Apr 13, 2018

@sguthals

This comment has been minimized.

Show comment Hide comment
@sguthals

sguthals Apr 16, 2018

Contributor

Agreed @donokuda!

I like this as a first step. Along with metrics, we can get a better workflow/discoverability for the entire extension related to PRs and future features (e.g. Issues)!
Contributor

sguthals commented Apr 16, 2018

Agreed @donokuda!

I like this as a first step. Along with metrics, we can get a better workflow/discoverability for the entire extension related to PRs and future features (e.g. Issues)!

jcansdale added some commits Apr 17, 2018

@jcansdale
Merge branch 'master' into enhancement/1591-status-bar-open-PRs
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
18b1ce1
@jcansdale
Add NumberOfOpenPullRequests counter to UsageModel 
Track usage of the PR status bar commands explicitly.
Loading status checks…
56844b1

@jcansdale jcansdale dismissed donokuda’s stale review via 56844b1 Apr 17, 2018

jcansdale added some commits Apr 17, 2018

@jcansdale
Make CA happy
Loading status checks…
0eeb81a
@jcansdale
Rename counter to NumberOfStatusBarOpenPullRequestList
Loading status checks…
01e5f7b
@jcansdale
Merge branch 'master' into enhancement/1591-status-bar-open-PRs
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e948886
@meaghanlewis

This comment has been minimized.

Show comment Hide comment
@meaghanlewis

meaghanlewis Apr 17, 2018

Contributor

This LGTM ✨
Contributor

meaghanlewis commented Apr 17, 2018

This LGTM ✨

@meaghanlewis meaghanlewis added the verified label Apr 17, 2018

@jcansdale
Merge branch 'master' into enhancement/1591-status-bar-open-PRs
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
40997e3
@grokys

grokys reviewed Apr 17, 2018
View changes

Looks good! Just one question.

src/GitHub.InlineReviews/Views/PullRequestStatusView.xaml
+ <Setter Property="Control.Visibility" Value="Collapsed" />
+ <Style.Triggers>
+ <!-- Visible when Number is Null -->
+ <DataTrigger Binding="{Binding Number}" Value="{x:Null}">

This comment has been minimized.

Show comment Hide comment
@grokys

grokys Apr 17, 2018

Contributor

Why not use a binding with TargetNullValue here as below?
@grokys

grokys Apr 17, 2018

Contributor

Why not use a binding with TargetNullValue here as below?

This comment has been minimized.

Show comment Hide comment
@jcansdale

jcansdale Apr 17, 2018

Contributor

I need this one to be Collapsed if it's not null. I'm setting it to Collapsed and overriding with Visible when it's null. Do you know if there's a simpler way to achieve this?
@jcansdale

jcansdale Apr 17, 2018

Contributor

I need this one to be Collapsed if it's not null. I'm setting it to Collapsed and overriding with Visible when it's null. Do you know if there's a simpler way to achieve this?
@grokys

grokys approved these changes Apr 17, 2018
View changes

@jcansdale jcansdale merged commit 44ebe6a into master Apr 17, 2018
2 checks passed

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

2.4.4 automation moved this from In progress to Done Apr 17, 2018

@jcansdale jcansdale deleted the enhancement/1591-status-bar-open-PRs branch Apr 17, 2018

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