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.

Sign up for GitHub

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

Jump to bottom

Allow selecting multiple files from left menu for deleting #426

Merged
merged 11 commits into from
Jul 1, 2024
Merged

Allow selecting multiple files from left menu for deleting #426

merged 11 commits into from
Jul 1, 2024

Conversation

guergana
Copy link
Collaborator

@guergana guergana commented Jun 14, 2024 •
edited
Loading

@guergana guergana marked this pull request as draft June 14, 2024 13:04
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented Jun 14, 2024 •
edited
Loading

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 62d01f8
Status: ✅  Deploy successful!
Preview URL: https://741b1078.opendataeditor.pages.dev
Branch Preview URL: https://341-new-approach.opendataeditor.pages.dev

View logs

@guergana guergana mentioned this pull request Jun 14, 2024
Closed
@guergana guergana changed the title 341 new approach Allow selecting multiple files from left menu for deleting Jun 14, 2024
@guergana guergana self-assigned this Jun 14, 2024
@guergana guergana marked this pull request as ready for review June 14, 2024 19:52
@guergana guergana requested review from roll and pdelboca June 14, 2024 19:53
roll
roll reviewed Jun 17, 2024
View reviewed changes
Copy link
Collaborator

@roll roll left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@guergana
Great it works for deletion!

I'm not sure if it's related to this change or non-related regression but when you add a new file it doesn't open it

@guergana
Copy link
Collaborator Author

@guergana
Great it works for deletion!

I'm not sure if it's related to this change or non-related regression but when you add a new file it doesn't open it

This task confirms that we need to start adding tests soon. Thanks for checking it. I will look into it.

@guergana
Copy link
Collaborator Author

@guergana Great it works for deletion!

I'm not sure if it's related to this change or non-related regression but when you add a new file it doesn't open it

I have tested and in the main branch the file opens only when only one file is added, when more files are added, no changes happen in the content panel. I dont know if it was like this before... I have pushed a commit that replicates what i see currently in main. Can you test again @roll ? 🙏

@guergana guergana requested a review from roll June 17, 2024 14:19
@roll
Copy link
Collaborator

roll commented Jun 18, 2024

Hi @guergana, have you pushed the commit to the remote branch?

@guergana
Copy link
Collaborator Author

Hi @guergana, have you pushed the commit to the remote branch?

Oh. I was having problems with my connection yesterday. Will try again. Thanks for the heads up

@guergana
Copy link
Collaborator Author

Hi @guergana, have you pushed the commit to the remote branch?

@roll Done.. now the push went through. :)

@pdelboca
Copy link
Member

@guergana this is working properly in my machine. I would suggest one extra change in the confirm dialog:
image

Maybe it is worth to list all files?

Or changing it to something like: Are you sure you want to delete all selected files? When we are about to delete multiple ones.

cc @romicolman

@romicolman
Copy link
Collaborator

Hi all! Let's go with @pdelboca's option:

"Are you sure you want to delete all selected files?" - When selecting more than one.
"Are you sure you want to delete this file?" - When selecting more than one.

I also tested the message when deleting folders and it's OK: "Are you sure you want to delete this folder?"

However, I think we will have a problem whenever the user selects a folder an let's say a couple of files outside the folder.

The message should be:

Are you sure you want to delete all selected elements?

@guergana let me know if I should create a separate ticket to adjust all messages.

@guergana
Copy link
Collaborator Author

Hi all! Let's go with @pdelboca's option:

"Are you sure you want to delete all selected files?" - When selecting more than one.
"Are you sure you want to delete this file?" - When selecting more than one.

I also tested the message when deleting folders and it's OK: "Are you sure you want to delete this folder?"

However, I think we will have a problem whenever the user selects a folder an let's say a couple of files outside the folder.

The message should be:

Are you sure you want to delete all selected elements?

@guergana let me know if I should create a separate ticket to adjust all messages.

@romicolman No, I think changing the messages belongs in this change. Will do it later. :)

@guergana
Copy link
Collaborator Author

@romicolman one final test, please. I have changed some things. I have tested but could you make a final test?

@guergana
Copy link
Collaborator Author

@pdelboca could you check the code again? I have rewritten the PR since your approval because deleting a folder and a file at the same time wasn't working, so now I have unified the logic into one dialog. 🙏

guergana added 8 commits July 1, 2024 13:21
@guergana
Selecting multiple files for deleting on left menu
2d2ebb2 
Attempt with new approach

Add selectedMultiplePaths state variable

Add selected case

Clear selectedMultiPaths when only a file is selected

Fix expanding folder

Remove leftover changes

Add case for when selectedFile is a string, not an array

Fix style errors
@guergana
Change messages for multiple folders and files
1ee4c36
@guergana
Selecting multiple files for deleting on left menu
c53b94c
@guergana
Attempt with new approach
abfe78e
@guergana
Add selectedMultiplePaths state variable
59d2c71
@guergana
Add selected case
433c489
@guergana
Remove leftover changes
b3625df
@guergana
Create only one element to delete files and folders
6eead5e
@guergana guergana force-pushed the 341-new-approach branch from ee2601f to 6eead5e Compare July 1, 2024 11:22
@guergana
Copy link
Collaborator Author

guergana commented Jul 1, 2024

Hello, @roll the recent store migration has created problems for me to merge this issue. Could you help me fix the merge errors?

roll
roll reviewed Jul 1, 2024
View reviewed changes
Copy link
Collaborator

@roll roll left a comment

Choose a reason for hiding this comment

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

Thanks @guergana !

I'm going to test it now

client/components/Application/Dialogs/DeleteFilesFolders.tsx Outdated Show resolved Hide resolved
client/store/actions/file.ts Show resolved Hide resolved
roll
roll approved these changes Jul 1, 2024
View reviewed changes
Copy link
Collaborator

@roll roll left a comment

Choose a reason for hiding this comment

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

Functionally-wise -- works for me 👍

@guergana guergana merged commit b6f2c1e into main Jul 1, 2024
8 checks passed
@guergana guergana deleted the 341-new-approach branch July 1, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roll roll roll approved these changes

@pdelboca pdelboca pdelboca approved these changes

Assignees

@guergana guergana

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem selecting multiple files on the left menu
4 participants
@guergana @roll @pdelboca @romicolman