LycheeOrg / Lychee-front

JS implementation of Lychee frontend
https://lycheeorg.github.io/
MIT License
48 stars 53 forks source link

remove the contextMenu if the user is not admin and don't have upload… #283

Closed cshyam1892 closed 2 years ago

cshyam1892 commented 2 years ago

https://github.com/LycheeOrg/Lychee/issues/1007

ildyria commented 2 years ago

Hi, thank you for your pull request. you may want to do npm format to uniformize the formatting. :)

cshyam1892 commented 2 years ago

Hello, thanks for your time and the advice. I hope it's okay now.

nagmat84 commented 2 years ago

Is the title of the PR a misnomer and misleading?

Even if the user is not admin and has no upload rights, the context menu is still needed to, e.g., download a selection of photos as a ZIP archive. From a quick look at the source code it rather seems that the "add" button is removed, not the context menu which would make much more sense.

I haven't tested the PR yet. So I am not sure what really happens.

kamil4 commented 2 years ago

Thank you for the PR!

I would rather see this fixed in a more limited fashion, in header.js rather than lychee.js, since we already deal with that button a great deal over there.

I pushed my commit to your tree that does it that way. @cshyam1892, could you verify that it still fully fixes the original problem? Thanks!

cshyam1892 commented 2 years ago

Is the title of the PR a misnomer and misleading?

Now that you mention it, I think you're right. It should've been "remove the context menu from landing page that's shown to non-admin users that don't have upload rights."

Even if the user is not admin and has no upload rights, the context menu is still needed to, e.g., download a selection of photos as a ZIP archive. From a quick look at the source code it rather seems that the "add" button is removed, not the context menu which would make much more sense.

Yeah it's still needed but I don't think non-admin users with restricted access has download rights, but nonetheless the context menu exists(again back at the title being misnomer).

Also, at first I tried to empty the items array in contextMenu.js, if the user is not admin and don't have upload rights but it showed the button without any items and it looked kinda dirty. So I thought it'd be good if we just remove the button entirely, instead of empty button that does nothing.

I haven't tested the PR yet. So I am not sure what really happens.

Like I said it removes the button with menu that is unessential for restricted users with no upload rights.

nagmat84 commented 2 years ago

Why has the PR been closed? Did @kamil4 suggestion not work for you?

kamil4 commented 2 years ago

I'm reopening it since I assume it was closed by mistake.

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

cshyam1892 commented 2 years ago

Thank you for the PR!

I would rather see this fixed in a more limited fashion, in header.js rather than lychee.js, since we already deal with that button a great deal over there.

I pushed my commit to your tree that does it that way. @cshyam1892, could you verify that it still fully fixes the original problem? Thanks!

Thanks for that. I think it fully fixes the original problem as the menu doesn't appear even though the user clicks through the album and comes back at the root of the gallery.

cshyam1892 commented 2 years ago

I'm reopening it since I assume it was closed by mistake.

Thanks!

kamil4 commented 2 years ago

It looks like you pushed a massive unrelated (?) commit. Could you keep this branch clean so that it can be merged easily?

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

cshyam1892 commented 2 years ago

It looks like you pushed a massive unrelated (?) commit. Could you keep this branch clean so that it can be merged easily?

Sorry about that, done now.