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

Add Editing for Link Share Labels #21357

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Conversation

gary-kim
Copy link
Member

@gary-kim gary-kim commented Jun 10, 2020

Closes #21265

Still a work in progress.
Will need to be added separately to the mobile and desktop apps.

This is more just an idea at the moment so feel free to comment on what you think about the idea of having a "note to self" for shares. The idea is to have a place to put notes to yourself about the share. Found the need to do this often for marking down which link is shared where.

The goal changed slightly. A entire note to self section is overkill so we are now giving the user the ability to edit the label for link shares.

image

@gary-kim gary-kim force-pushed the feature/21265/personal-share-notes branch 4 times, most recently from 4286e9f to cd42f88 Compare June 10, 2020 10:12
@gary-kim
Copy link
Member Author

gary-kim commented Jun 10, 2020

Umm... confused. So, it passed but it also didn't?
Screenshot_2020-06-10 Build #29713 13 5 - nextcloud server - Drone CI

@gary-kim gary-kim force-pushed the feature/21265/personal-share-notes branch 3 times, most recently from 4709ec4 to 0dc383b Compare June 10, 2020 11:32
@gary-kim gary-kim added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 10, 2020
@gary-kim gary-kim force-pushed the feature/21265/personal-share-notes branch from 0dc383b to 264ce21 Compare June 10, 2020 11:44
@gary-kim gary-kim self-assigned this Jun 10, 2020
@jancborchardt
Copy link
Member

I’m not sure we should right away go with such a detailed note. One thing we should for sure already do is to have a subline for the "Copy link" which shows the details about chosen options, like:

Share link
Editable, password protected

or

Share link
Read-only, expires in 5 days

This already helps a lot to know what these links do and are for, without any further interaction needed.

@gary-kim
Copy link
Member Author

gary-kim commented Jun 11, 2020

I was implementing a solution like this because, for example, my resume has 3 different share links that I have sent to different people, all read only and with no expiry. I needed to remove one of them but I had no way to know which one was for whom. This happens pretty often where I share multiple links for each file or folder, each with the same settings but for different people, but really struggle to know which share link went to whom. When I want to remove them.

One idea, to maybe combine both options: could we have something where the note to self is more of a "share title"? If you don't put anything there, the message shown is what @jancborchardt came up with, but if you put a title, it shows that instead? Let me see if I can make a quick prototype of that idea.

So usually, it would say something like

Share link
Read-only, expires in 5 days

but if you put a message on it, for example, "for best friend", then it says:

Share link
for best friend

@gary-kim
Copy link
Member Author

gary-kim commented Jun 11, 2020

Oh wait, didn't notice this, there's a label system for link shares already. Could we maybe just expose editing that to the user?

if (this.share.label && this.share.label.trim() !== '') {
return this.share.label
}

// only link shares have labels
if ($share->getShareType() === IShare::TYPE_LINK && $label !== null) {
$share->setLabel($label);

cc @skjnldsv

@rullzer
Copy link
Member

rullzer commented Jun 11, 2020

Oh wait, didn't notice this, there's a label system for link shares already. Could we maybe just expose editing that to the user?

if (this.share.label && this.share.label.trim() !== '') {
return this.share.label
}

// only link shares have labels
if ($share->getShareType() === IShare::TYPE_LINK && $label !== null) {
$share->setLabel($label);

cc @skjnldsv

yes please!
Making this editable would be awesome!

The stuff that jan proposes is a next step I guess (having some context below the link)

@jancborchardt
Copy link
Member

Oh wait, didn't notice this, there's a label system for link shares already. Could we maybe just expose editing that to the user?

Oh cool, sure. :) Then we could just show that note in parentheses behind like:

Share link (for professor)

Share link (for Alex)

This would then also work for the future if we get that subline with the further info.
Let’s make it a single input field instead of textarea to force people to keep it short. :)

@gary-kim
Copy link
Member Author

gary-kim commented Jun 11, 2020

Let’s make it a single input field instead of textarea to force people to keep it short. :)

To do that properly, a change will be required in nextcloud-vue as well. I'll get on that.

Also, I'm definitely not going to keep my messages short :)

@jancborchardt
Copy link
Member

Also, I'm definitely not going to keep my messages short :)

Hehe ;D You’re free to do that of course, I just think that we have to keep in mind where they would need to be displayed – and those will likely be 1-line displays only mostly. Of course people can write more in an input field.

@gary-kim gary-kim added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 12, 2020
@gary-kim gary-kim force-pushed the feature/21265/personal-share-notes branch from 264ce21 to a3477c9 Compare June 12, 2020 12:30
@gary-kim
Copy link
Member Author

gary-kim commented Jun 12, 2020

Okay, this is what I've got now (turns out no upstream change is required 😄 ):
image

@gary-kim gary-kim removed the 2. developing Work in progress label Jun 12, 2020
@gary-kim gary-kim added this to the Nextcloud 20 milestone Jun 18, 2020
@tobiasKaminsky
Copy link
Member

Oh cool, sure. :) Then we could just show that note in parentheses behind like:

Share link (for professor)
Share link (for Alex)

Isn't this some kind of redundant? User sees via icon and as they created it themselves that it is a share link.
So I would instead let the user replace the full text.
This then also helps a bit with longer names.

Btw: I created a feature parity issue: #21593

@gary-kim
Copy link
Member Author

Thanks @tobiasKaminsky

Also, ping for reviews?

@jancborchardt
Copy link
Member

jancborchardt commented Jul 2, 2020

Isn't this some kind of redundant? User sees via icon and as they created it themselves that it is a share link.
So I would instead let the user replace the full text.

Remember that not everyone spends so much time in Nextcloud as we do. :) So I would still say we should keep the "Share link" text and only put the label inside parentheses behind that.

That is, @gary-kim in your example the "for" is outside of the parentheses but it should not be, basically it’s:

Share link (custom text here)

@gary-kim gary-kim force-pushed the feature/21265/personal-share-notes branch from 6ab1aa7 to af08f70 Compare July 3, 2020 05:48
@gary-kim
Copy link
Member Author

gary-kim commented Jul 3, 2020

That is, @gary-kim in your example the "for" is outside of the parentheses but it should not be, basically it’s:

Share link (custom text here)

Okie, fixed 👍

@tobiasKaminsky
Copy link
Member

On Android it looks like this:
image

(with primary color themed to black)

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.

Looks good design-wise! :)

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comments :)

@gary-kim gary-kim force-pushed the feature/21265/personal-share-notes branch from af08f70 to 7820e83 Compare July 8, 2020 18:24
@gary-kim gary-kim requested a review from skjnldsv July 8, 2020 18:27
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Good stuff! Seems to do the trick. I'm all for getting in and fixing small stuff later in followups!

@gary-kim gary-kim force-pushed the feature/21265/personal-share-notes branch from 7820e83 to 52b1b28 Compare July 10, 2020 23:04
@gary-kim
Copy link
Member Author

Okay, otherwise good?

Signed-off-by: Gary Kim <gary@garykim.dev>
@gary-kim gary-kim force-pushed the feature/21265/personal-share-notes branch from 52b1b28 to a2cedce Compare July 14, 2020 02:02
@gary-kim
Copy link
Member Author

Rebased

@gary-kim gary-kim added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 14, 2020
@rullzer rullzer merged commit 3a1e907 into master Jul 14, 2020
@rullzer rullzer deleted the feature/21265/personal-share-notes branch July 14, 2020 07:24
@jospoortvliet
Copy link
Member

Great work @gary-kim - I think this will make a lot of users happy ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Personal Share Notes
6 participants