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

Refactored Clone Dialog #1787

Merged
merged 22 commits into from Sep 11, 2018

Conversation

Projects
None yet
5 participants
@grokys
Contributor

grokys commented Jul 19, 2018

Refactored the clone dialog, basing it initially on the clone dialog from desktop:

Features

  • Now uses GraphQL to load the list of repositories. This improves the load time of the list.
  • Separate GitHub and Enterprise tabs
  • A new "Clone from URL" tab

Screenshots

Particularly the URL clone view needs some input from @donokuda.

image

image

Scopes

GraphQL requires the read:org scope the access the user's repositories. This means that the user will have to log out and back in again to use the clone dialog. The following message will be shown in this case (needs input from @donokuda):

image

Caveats

  • You currently need to log out and back in because the GraphQL query needs the read:org scope which your previous login won't have. Need to implement a "You need to log out and back in again" flow in the dialog for this. This is now implemented
  • Only showing first 100 org repositories because of octokit/octokit.graphql.net#132 now fixed.

Depends on #1761

Closes #1757

Initial reimplementation of Clone dialog.
- Uses GraphQL
- Has a page for cloning from URL

@donokuda donokuda self-assigned this Jul 19, 2018

Return user's repositories and group in list.
Previously were just returning repositories that belong to orgs. Now return the user's owned repositories as well, and group by owner in the list.

Currently returning only first 100 org repositories because of octokit/octokit.graphql.net#132 .

@grokys grokys added the WIP label Jul 20, 2018

@meaghanlewis meaghanlewis added this to the 2.5.5 milestone Jul 30, 2018

@donokuda

This comment has been minimized.

Show comment
Hide comment
@donokuda

donokuda Jul 30, 2018

Member

You currently need to log out and back in because the GraphQL query needs the read:org scope which your previous login won't have. Need to implement a "You need to log out and back in again" flow in the dialog for this.

In case it hasn't been considered yet, a potentially "quick & dirty" flow for this is to pop up a modal dialog on top of the clone dialog that give the individual the option to log out.

I'd imagine the flow to break down like this:

  1. I click on "Clone" from the GitHub section of Team Explorer's connect page
  2. A dialog appears explaining that I need to log out in order to proceed with two options: [Cancel] [Log out]
  3. I click "Log out," the extension attempts to log me out.
  • On success : the login modal appears and I step through that flow. (Ideally: After logging in, I'm presented with the clone dialog since that's the action I've been meaning to do in the first place.)
  • On failure : Another dialog appears with the error and two options: [Cancel] [Try again]
    • Selecting "Try again" attempts logging out again.
Member

donokuda commented Jul 30, 2018

You currently need to log out and back in because the GraphQL query needs the read:org scope which your previous login won't have. Need to implement a "You need to log out and back in again" flow in the dialog for this.

In case it hasn't been considered yet, a potentially "quick & dirty" flow for this is to pop up a modal dialog on top of the clone dialog that give the individual the option to log out.

I'd imagine the flow to break down like this:

  1. I click on "Clone" from the GitHub section of Team Explorer's connect page
  2. A dialog appears explaining that I need to log out in order to proceed with two options: [Cancel] [Log out]
  3. I click "Log out," the extension attempts to log me out.
  • On success : the login modal appears and I step through that flow. (Ideally: After logging in, I'm presented with the clone dialog since that's the action I've been meaning to do in the first place.)
  • On failure : Another dialog appears with the error and two options: [Cancel] [Try again]
    • Selecting "Try again" attempts logging out again.

@meaghanlewis meaghanlewis modified the milestones: 2.5.5, 2.5.6 Aug 13, 2018

grokys added some commits Aug 27, 2018

Merge branch 'refactor/sdk-csproj' into refactor/clone
 Conflicts:
	src/GitHub.App/Resources.resx
	src/GitHub.VisualStudio.UI/Properties/AssemblyInfo.cs
Address TODOs.
Now that we've updated Octokit.GraphQL.
Return LoginResult from LoginManager.
So we can return the scopes as well as the user.

meaghanlewis and others added some commits Sep 5, 2018

@meaghanlewis

This comment has been minimized.

Show comment
Hide comment
@meaghanlewis

meaghanlewis Sep 6, 2018

Contributor

This looks great to me @grokys !

One thing that I ran into was when I wanted to enter the username and repo to clone - ex github/quality. I wasn't signed into my GitHub account at the time - only GitHub Enterprise. This login dialog appeared:

screen shot 2018-09-06 at 2 29 11 pm

I authenticated with it, but it didn't actually sign me in and I got an error like this Error encountered while cloning the remote repository: Git failed with a fatal error. unable to access 'https://github.com/github/quality/': The requested URL returned error: 403.

Is it possible to show our Sign in dialog here instead to avoid this from possibly confusing users?

Contributor

meaghanlewis commented Sep 6, 2018

This looks great to me @grokys !

One thing that I ran into was when I wanted to enter the username and repo to clone - ex github/quality. I wasn't signed into my GitHub account at the time - only GitHub Enterprise. This login dialog appeared:

screen shot 2018-09-06 at 2 29 11 pm

I authenticated with it, but it didn't actually sign me in and I got an error like this Error encountered while cloning the remote repository: Git failed with a fatal error. unable to access 'https://github.com/github/quality/': The requested URL returned error: 403.

Is it possible to show our Sign in dialog here instead to avoid this from possibly confusing users?

@grokys

This comment has been minimized.

Show comment
Hide comment
@grokys

grokys Sep 10, 2018

Contributor

@meaghanlewis hmm yes, I'm not quite sure how to deal with that. If the repository is publicly visible then this clone should actually work. We could make a check to see if we have a connection to the server that the clone is being requested from, but again, the clone may actually work.

I notice desktop does the same thing in this situation and shows a login dialog. I'm tempted to say that this is expected behavior.

Contributor

grokys commented Sep 10, 2018

@meaghanlewis hmm yes, I'm not quite sure how to deal with that. If the repository is publicly visible then this clone should actually work. We could make a check to see if we have a connection to the server that the clone is being requested from, but again, the clone may actually work.

I notice desktop does the same thing in this situation and shows a login dialog. I'm tempted to say that this is expected behavior.

@jcansdale

jcansdale approved these changes Sep 10, 2018 edited

Sorry, wrong PR. 😊

@jcansdale

Changing Approve to Comment.

@meaghanlewis

This comment has been minimized.

Show comment
Hide comment
@meaghanlewis

meaghanlewis Sep 10, 2018

Contributor

hey @grokys im okay to leave the behavior the same when trying to clone a private repo while signed out of a GitHub account. This might just be an edge case and there is a workaround anyway to sign into the account first and then clone.

Contributor

meaghanlewis commented Sep 10, 2018

hey @grokys im okay to leave the behavior the same when trying to clone a private repo while signed out of a GitHub account. This might just be an edge case and there is a workaround anyway to sign into the account first and then clone.

@meaghanlewis

This comment has been minimized.

Show comment
Hide comment
@meaghanlewis

meaghanlewis Sep 10, 2018

Contributor

One more question - could you add a message for when someone tries to clone a repo they already have?

Currently, the clone button is disabled when trying to clone an existing repo but it might not be clear why.
screen shot 2018-09-10 at 8 01 53 am

In the old (current) clone dialog we at least showed an icon in the path to indicate it can't be cloned to that location.
screen shot 2018-09-10 at 8 10 06 am

And in desktop, they have an explicit message saying the destination already exists. Could we also show a message similar to this?
screen shot 2018-09-10 at 8 01 39 am

Contributor

meaghanlewis commented Sep 10, 2018

One more question - could you add a message for when someone tries to clone a repo they already have?

Currently, the clone button is disabled when trying to clone an existing repo but it might not be clear why.
screen shot 2018-09-10 at 8 01 53 am

In the old (current) clone dialog we at least showed an icon in the path to indicate it can't be cloned to that location.
screen shot 2018-09-10 at 8 10 06 am

And in desktop, they have an explicit message saying the destination already exists. Could we also show a message similar to this?
screen shot 2018-09-10 at 8 01 39 am

Include InfoPanel template.
To make error messages appear in clone dialog.

@grokys grokys changed the title from WIP: Refactored Clone Dialog to Refactored Clone Dialog Sep 11, 2018

@grokys grokys merged commit 0e64838 into master Sep 11, 2018

2 checks passed

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

@meaghanlewis meaghanlewis removed the WIP label Sep 26, 2018

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