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

Show lock owner in tooltip #12

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Show lock owner in tooltip #12

merged 1 commit into from
Dec 17, 2019

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Nov 28, 2019

Implements #7

@jancborchardt
Copy link
Member

jancborchardt commented Nov 29, 2019

See my comment in #3 (comment) :)

  • 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
Copy link
Member Author

juliusknorr commented Nov 29, 2019

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

  • On click we show the contacts menu
  • Locking/unlocking will be performed though the 3-dots menu as it is currently
  • A tooltip will show who and when the file was locked
  • In the grid view, we just show the icon-password above the 3-dots menu as it is currently

@jancborchardt
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

@juliushaertl screenshot seems good! 👍 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
Copy link
Member

cc @daita @karlitschek also for reference and review

@karlitschek
Copy link
Member

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

@karlitschek
Copy link
Member

@juliushaertl @rullzer @daita Can we merge that?

@rullzer
Copy link
Member

rullzer commented Dec 17, 2019

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 juliusknorr marked this pull request as ready for review December 17, 2019 14:19
@juliusknorr
Copy link
Member Author

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

image

@jancborchardt
Copy link
Member

jancborchardt commented Dec 17, 2019

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"

Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Copy link
Member Author

Order is changed as well.

@jancborchardt jancborchardt added the enhancement New feature or request label Dec 17, 2019
@jancborchardt jancborchardt added this to the Nextcloud 18 milestone Dec 17, 2019
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Nice! :)

@rullzer
Copy link
Member

rullzer commented Dec 17, 2019

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

@ArtificialOwl
Copy link
Member

tested and all, thanks @juliushaertl

@ArtificialOwl ArtificialOwl merged commit 02f9646 into master Dec 17, 2019
@delete-merged-branch delete-merged-branch bot deleted the enh/7/show-user branch December 17, 2019 21:12
@rullzer
Copy link
Member

rullzer commented Dec 18, 2019

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

@rullzer
Copy link
Member

rullzer commented Dec 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants