Skip to content
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

Implement ui to support multiple remotes #1146

Merged
merged 30 commits into from
Aug 11, 2022

Conversation

BoscoCHW
Copy link
Contributor

@BoscoCHW BoscoCHW commented Jul 15, 2022

@fcollonval

I implemented a dialogue to manage remote repositories.

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch BoscoCHW/jupyterlab-git/issue-1142

@BoscoCHW BoscoCHW changed the title Implement ui to add and remove multiple remotes Implement ui to support multiple remotes Jul 19, 2022
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this one. This looks pretty good.

I added some suggestions. As discussed at the meeting, I let you propose something to display the dialog only if the branch has no remote or when triggered from the menu.

jupyterlab_git/handlers.py Outdated Show resolved Hide resolved
jupyterlab_git/handlers.py Outdated Show resolved Hide resolved
jupyterlab_git/handlers.py Outdated Show resolved Hide resolved
src/components/AddRemoteDialogue.tsx Outdated Show resolved Hide resolved
src/components/AddRemoteDialogue.tsx Outdated Show resolved Hide resolved
src/components/AddRemoteDialogue.tsx Outdated Show resolved Hide resolved
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BoscoCHW

Nice work of getting a pure React and a widget dialogs within this PR.

On the code side, would you mind adding tests (in particular Python tests for the new handlers).

On the design side,

What about renaming the menu entry Manage Remote Repositories instead of Add Remote Repository?
About the remote dialog:
image

What about moving the Add button below the input fields and remove the footer (with the two buttons)?
As it takes some time to retrieve the remote list, what about differentiating state.existingRemotes === null and state.existingRemotes.length === 0. So you can inform the user the remotes list is being retrieve when the state is null. And that there is no remote if the list is empty?

For the advanced push dialog, could you also inform the user that the remote list is being retrieved to reduce the popping of the list?

@fcollonval fcollonval marked this pull request as ready for review July 28, 2022 14:49
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BoscoCHW

I propose to do a bit more refactoring of the command. But every is there.

We can discuss this PR more tomorrow.

src/components/AddRemoteDialogue.tsx Outdated Show resolved Hide resolved
@@ -254,8 +255,8 @@ export function addCommands(

/** Command to add a remote Git repository */
commands.addCommand(CommandIDs.gitAddRemote, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's refactor this a bit. Would you mind renaming the gitAddRemote command to be gitManageRemote.

We then should remove the ability to pass args to the execute method. Then in the dialog we should do the addition without closing the dialog. So the dialog is respecting its contract of management not of single action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the command to gitManageRemote and disabled arguments for the command.
To-do:
Implement adding a new remote without closing the dialog

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clean-up the tests and adding new one @BoscoCHW

I think is time to get this in 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants