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

Support multi-selection in file lists #1136

Merged
merged 61 commits into from
Jul 28, 2022
Merged

Conversation

BoscoCHW
Copy link
Contributor

@BoscoCHW BoscoCHW commented Jun 22, 2022

Fixes #379

Implement Control- and Shift-Click file selection
Shift-click behaviour is not ideal. Failed to simulate shift-click behaviour on Google Drive.
Issues:

  1. Can't deselect when shift click direction changes

Other issues:

  1. IStatusFile object equality checking leads to a lot of duplicate codes
  2. File status changes is not immediately reflected in FileList state, leading to invalid commands in context menu

Shift-click behaviour is not ideal. Failed to simulate shift-click behaviour on Google Drive.
Issues:
1. Can't deselect when shift click direction changes

Other issues:
1. IStatusFile object equality checking leads to a lot of duplicate codes
2. File status changes is not immediately reflected in FileList state, leading to invalid commands in context menu
@github-actions
Copy link

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

@BoscoCHW BoscoCHW marked this pull request as ready for review June 22, 2022 20:56
@BoscoCHW
Copy link
Contributor Author

@fcollonval
I think the multiple file selection for normal staging is complete. I just need to fix the tests. Please let me know if you want to make any changes to the code. Thank you!

@fcollonval fcollonval changed the title Issue 379 Support multi-selection in file lists Jun 27, 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 a lot @BoscoCHW

I have a couple of suggestions - mainly related to API and algorithm simplification.

On Thursday, I will explain how to create integration tests for this PR.

src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileItem.tsx Outdated Show resolved Hide resolved
src/components/FileList.tsx Outdated Show resolved Hide resolved
src/components/FileList.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

We are almost there. I have made some additional suggestions. And I found a bug. In simple staging mode, when clicking on a file item button the mark status changed (it should not). This can be solved by stopping the event propagation.

src/components/FileList.tsx Show resolved Hide resolved
src/components/GitPanel.tsx Outdated Show resolved Hide resolved
src/components/GitStage.tsx Outdated Show resolved Hide resolved
src/components/GitStage.tsx Outdated Show resolved Hide resolved
src/components/SelectAllButton.tsx Outdated Show resolved Hide resolved
ui-tests/README.md Show resolved Hide resolved
ui-tests/tests/file-selection.spec.ts Outdated Show resolved Hide resolved
BoscoCHW and others added 7 commits July 21, 2022 12:14
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.

Let's add some doc strings then merge 😉

@@ -41,6 +41,8 @@ export interface IGitStageProps {
* Row renderer
*/
rowRenderer: (props: ListChildComponentProps) => JSX.Element;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* Optional select all element
*/

}
};

markUntilFile = (file: Git.IStatusFile): void => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
markUntilFile = (file: Git.IStatusFile): void => {
/**
* Mark files from the latest selected to this one
*
* @param file The current clicked-on file
*/
markUntilFile = (file: Git.IStatusFile): void => {

Comment on lines 314 to 315
* @param file
* @param options
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param file
* @param options
* @param file The current clicked-on file
* @param options Selection options

}
};

toggleAllFiles = (files: Git.IStatusFile[]): void => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
toggleAllFiles = (files: Git.IStatusFile[]): void => {
/**
* Set mark status from all selected button
*
* @param files Files to toggle
*/
toggleAllFiles = (files: Git.IStatusFile[]): void => {

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 a lot. Let's merge and release 😉

@fcollonval fcollonval merged commit 2091f48 into jupyterlab:master Jul 28, 2022
@fcollonval
Copy link
Member

@all-contributors please add @BoscoCHW for code

@allcontributors
Copy link
Contributor

@fcollonval

I've put up a pull request to add @BoscoCHW! 🎉

@fcollonval
Copy link
Member

@all-contributors please add @iflinda for code

@allcontributors
Copy link
Contributor

@fcollonval

I've put up a pull request to add @iflinda! 🎉

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

Successfully merging this pull request may close these issues.

[UX] Unable to select multiple files to stage at once
3 participants