search

okfn / opendataeditor

No-code application to explore and publish all kinds of data: datasets, tables, charts, maps, stories, and more. Forever free and open source project powered by open standards and generative AI.
http://opendataeditor.okfn.org
MIT License
149 stars 18 forks source link

Allow selecting multiple files from left menu for deleting #426

Closed guergana closed 14 hours ago

guergana commented 2 weeks ago
cloudflare-pages[bot] commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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 ? :pray:

roll commented 1 week ago

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

guergana commented 1 week ago

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 commented 1 week ago

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

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

pdelboca commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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 commented 5 days ago

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

guergana commented 4 days ago

@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. :pray:

guergana commented 17 hours ago

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