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

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

Merged
merged 11 commits into from Apr 17, 2018

Conversation

Projects
5 participants
@jcansdale
Contributor

jcansdale commented Apr 10, 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

@sguthals

This comment has been minimized.

Contributor

sguthals commented 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 requested a review from donokuda Apr 13, 2018

@donokuda

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.

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

Add NumberOfOpenPullRequests counter to UsageModel
Track usage of the PR status bar commands explicitly.

jcansdale added some commits Apr 17, 2018

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Apr 17, 2018

This LGTM

@grokys

Looks good! Just one question.

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

@grokys

grokys Apr 17, 2018

Contributor

Why not use a binding with TargetNullValue here as below?

This comment has been minimized.

@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

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

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