search

nextcloud / files_lock

GNU Affero General Public License v3.0
25 stars 8 forks source link

Show lock owner in tooltip #12

Closed juliusknorr closed 4 years ago

juliusknorr commented 4 years ago

Implements #7

jancborchardt commented 4 years ago

See my comment in https://github.com/nextcloud/files_lock/issues/3#issuecomment-551121951 :)

  • In the menu it then says in the entry "Unlock" and in a subline: "Locked 16m ago by Frank" so people have context.
  • Ideally we have a way for people to then communicate too, e.g. either unlock it or send a message on Talk before. (Could do via modal when you click the "Unlock" entry?)

The unlock entry could then have the avatar of the person instead of the lock as icon.

juliusknorr commented 4 years ago

In the menu it then says in the entry "Unlock" and in a subline: "Locked 16m ago by Frank" so people have context.

Not possible with the current file actions API. We can just set the label itself, but that doesn't look good imo: image

The unlock entry could then have the avatar of the person instead of the lock as icon.

I don't think that would be a good solution as just showing an avatar gives no idea that the avatar is actually about locking.

How about this: image

jancborchardt commented 4 years ago

Not possible with the current file actions API. We can just set the label itself, but that doesn't look good imo:

Is it possible to add a second action, so a second line? Also because for a locked file, a lot of the actions like move, rename, etc should probably be disabled anyway, right?

juliusknorr commented 4 years ago

Is it possible to add a second action, so a second line?

Yes it would look like this: image

How would that integrate the contacts menu then? Having another popover on top of the existing one doesn't seem nice to me.

Also because for a locked file, a lot of the actions like move, rename, etc should probably be disabled anyway, right?

Not possible as far as I can see right now. We cannot disable the core file actions from outside of their apps javascript context.

jancborchardt commented 4 years ago

@juliushaertl screenshot seems good! :+1: Can we get the time as well, like "Locked by admin 4 min ago"?

Or what do you think?

How would that integrate the contacts menu then? Having another popover on top of the existing one doesn't seem nice to me.

We do have that in the header Contacts menu as well – having a menu inside a popover is just something we will likely have, and it’s fine. In any case, it’s a different discussion?

jancborchardt commented 4 years ago

cc @daita @karlitschek also for reference and review

karlitschek commented 4 years ago

super cool. I agree that the time would be very good no know

karlitschek commented 4 years ago

@juliushaertl @rullzer @daita Can we merge that?

rullzer commented 4 years ago

Lets be practical here. Merge this as it is already a nice improvement and get it out. And then for the next version we add the tooltip with text. Small PRs are good PRs ;)

juliusknorr commented 4 years ago

Polished up a bit, ready to review from my side:

image

jancborchardt commented 4 years ago

If the entries are switched around then it’s good to go!

So you have the info first: "Locked by Roeland" and then the action "Unlock file"

juliusknorr commented 4 years ago

Order is changed as well.

rullzer commented 4 years ago

@daita can you give this a go as well and merge?

ArtificialOwl commented 4 years ago

tested and all, thanks @juliushaertl

rullzer commented 4 years ago

Ah mmm so it does :boom: on the context menu. Because if a file is not locked the displayname is undefined. Which in turn breaks the sort.

rullzer commented 4 years ago

https://github.com/nextcloud/files_lock/pull/12/files#diff-d8804ee2bfb753a4105404489eded466R68-R75

There to be exact