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 Filetree context menu #528

Merged
merged 24 commits into from
Sep 24, 2024
Merged

Implement Filetree context menu #528

merged 24 commits into from
Sep 24, 2024

Conversation

guergana
Copy link
Collaborator

@guergana guergana commented Aug 26, 2024

Note: since the context menu conflicts design-wise with the multiselect, we will disable it in #543

@guergana guergana changed the title Implement filetree context menu Implement Filetree context menu Aug 26, 2024
Copy link

cloudflare-workers-and-pages bot commented Aug 29, 2024

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 58fdd96
Status: ✅  Deploy successful!
Preview URL: https://af31a46e.opendataeditor.pages.dev
Branch Preview URL: https://511-context-menu.opendataeditor.pages.dev

View logs

@guergana guergana changed the title Implement Filetree context menu [WiP] Implement Filetree context menu Sep 8, 2024
@guergana guergana changed the title [WiP] Implement Filetree context menu Implement Filetree context menu Sep 13, 2024
@pdelboca
Copy link
Member

This is looking good @guergana !!

The "Do not show this again" does not seem to work. It keeps appearing every time I upload a file.

Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

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

I checked it on Mac. I agree that there is still something going on the context menu within folders.

There is another problem. Faith adapted the original context menu to make it more generic (check comment added on the ticket a couple of weeks ago). We need to implement the generic option because we will have inconsistencies, like this one:

Captura de pantalla 2024-09-16 a la(s) 10 31 52 a  m

@guergana
Copy link
Collaborator Author

I checked it on Mac. I agree that there is still something going on the context menu within folders.

There is another problem. Faith adapted the original context menu to make it more generic (check comment added on the ticket a couple of weeks ago). We need to implement the generic option because we will have inconsistencies, like this one:
Captura de pantalla 2024-09-16 a la(s) 10 31 52 a  m

What I did for this case is to open the folder. @romicolman I have pushed a new commit with this solution:

image

What do you think?

@romicolman
Copy link
Collaborator

Hi! I think there is a confusion. Right now, I see this:

  1. Open Folder Location - The ODE folder where this file exists

The description creates confusion since we are distinguishing between folders and files in the app and in the sentence we are mixing both elements (The ODE FOLDER where this FILE exists)

  1. Delete file - Only removes this file from the ODE folder

Again, mixed language.

This is my suggestion:

Add the same text for context menu on files and folders:

  1. Open Location - The ODE folder where this element exists
  2. Delete - Removes this element from the ODE folder

@guergana
Copy link
Collaborator Author

Hi! I think there is a confusion. Right now, I see this:

1. Open Folder Location - The ODE folder where this file exists

The description creates confusion since we are distinguishing between folders and files in the app and in the sentence we are mixing both elements (The ODE FOLDER where this FILE exists)

2. Delete file - Only removes this file from the ODE folder

Again, mixed language.

This is my suggestion:

Add the same text for context menu on files and folders:

1. Open Location - The ODE folder where this element exists

2. Delete - Removes this element from the ODE folder

oh, yes, i missed one of the names. @romicolman could you check again?

Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

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

It looks good! Thanks!!

@guergana guergana merged commit 1bfca4d into main Sep 24, 2024
9 checks passed
@guergana guergana deleted the 511-context-menu branch September 24, 2024 07:56
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.

Implement context menu - Files and folders
3 participants