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

allow to create multiple link shares via share api #11844

Merged
merged 14 commits into from
Nov 1, 2018

Conversation

schiessle
Copy link
Member

@schiessle schiessle commented Oct 15, 2018

  • Allow to create multiple share links (API level)
  • Get multiple share links (API level)
  • allow to give share links unique names (API level)
  • allow to change unique name of a share (API level)
  • Allow to create multiple share links (web ui)
  • Show multiple share links (web ui)
  • allow to give share links unique names (web ui)
  • allow to change unique name of a share (web ui)

for ux discussion, etc see #11169
Fix #10434
Closes #11537 : not possible, we cannot wait for the request of a new share to be done as the .done or success hooks generate a new scope and a clipboard triggered by a click needs to be in a direct call of a function triggered by an event :(
Fix #11169

@schiessle
Copy link
Member Author

schiessle commented Oct 17, 2018

cc @AndyScherzinger @tobiasKaminsky @marinofaggiana @camilasan @rullzer With Nextcloud 15 we will support multiple share links. This PR already implements everything you need to start implementing it on the client side. Just in case you are looking for something to do 😉

Creating links works the same as before. There is just the additional parameter "label" you can use to create/update link shares so that the user can give the share link names to remember which link was used for what.

@marinofaggiana
Copy link
Member

ToDo added :-)

@jancborchardt
Copy link
Member

I’d suggest to split this up to prevent it from becoming too big of a pull request:

API stuff:

  • Allow to create multiple share links (API level)
  • Get multiple share links (API level)
  • allow to give share links unique names (API level)
  • allow to change unique name of a share (API level)

Basic interface stuff:

  • Allow to create multiple share links (web ui)
  • Show multiple share links (web ui)

Future interface stuff that is optional:

  • allow to give share links unique names (web ui)
  • allow to change unique name of a share (web ui)

Especially since you already mentioned the possible conflicts with #11537 (which is quite short so shouldn’t really conflict a lot.)

@jancborchardt
Copy link
Member

jancborchardt commented Oct 30, 2018

Just had a short call with @skjnldsv, and this is the spec now:

  1. The default share list has one entry in it called 🔗 Share link as usual. On the right there’s a ➕ icon.

    • We have no checkbox anymore. A link either exists or not.
  2. Clicking the whole row (not only the ➕) converts the entry into an active link

    • Automatically copies it to clipboard, with a tooltip as feedback "Link copied".
    • By default with read-only permissions (or edit? what do you think?)
    • It also replaces the ➕ icon with 📋 Copy link and 🞄🞄🞄 buttons.
    • Do we want to automatically open the 🞄🞄🞄 menu here too? (Maybe at least for the first link so it kind of shows people the options?)
  3. Clicking the 🞄🞄🞄 button reveals the menu we know with the adjustment options.

    • Additionally it will have an entry called ➕ Add another link (feel free to suggest better/more succinct wording)
    • Tapping that entry will create another entry below it, basically same as step 2 above, automatically activating it and copying etc.

That’s that for now. Did I forget anything @skjnldsv?
In the future we can also have renaming of links, but for now it will just be "Share link", "Share link 2", "Share link 3" etc. Or ideally "Share link (read-only)" and "Share link 2 (read-only)" and "Share link (editable)" and "Share link (password-protected)".

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@skjnldsv skjnldsv force-pushed the multiple-link-shares branch from b7bdd4b to 9503528 Compare October 31, 2018 07:27
@skjnldsv skjnldsv added the high label Oct 31, 2018
@skjnldsv
Copy link
Member

skjnldsv commented Oct 31, 2018

Should be finished today!
@jancborchardt @nextcloud/designers Can we try to find another saying for "Share link" I'm always finding this confusing as it looks like an action and not a statement :)

Can you start a review now?
That would increase my response time to fixes to day for the feature freeze :)

Basically the only thing left is the label handling! How should we do that?

  1. Add an edit button?
  2. In the popovermenu?

capture d ecran_2018-10-31_10-57-51

@jancborchardt
Copy link
Member

Just beginning to test this! :) First feedback:

  • The ➕ icon is not keyboard-accessible, no highlight on focus
  • Ideally the whole row should be clickable, not just the ➕
  • Not sure it’s screenreader-accessible right now with the "title" on the icon. But it would be when the whole row is the action.

@jancborchardt
Copy link
Member

Made the background of the share link primary color instead of bland grey. This is in line with the change in the Talk app at nextcloud/spreed#1274

@skjnldsv
Copy link
Member

Made the background of the share link primary color instead of bland grey. This is in line with the change in the Talk app at nextcloud/spreed#1274

I was afraid it would be too much, but it is spot on!! 👍

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 31, 2018
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 1, 2018
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Fixed

Not so fast; I have some complaints ;-)

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2018

@danxuliu be quick!
I want to get this in today! And don't use this as an excuse to merge your pr before mine :p

core/js/sharedialoglinkshareview.js Outdated Show resolved Hide resolved
core/js/tests/specs/sharedialoglinkshareview.js Outdated Show resolved Hide resolved
core/js/tests/specs/sharedialoglinkshareview.js Outdated Show resolved Hide resolved
core/js/tests/specs/sharedialoglinkshareview.js Outdated Show resolved Hide resolved
core/js/tests/specs/sharedialoglinkshareview.js Outdated Show resolved Hide resolved
core/js/tests/specs/sharedialogviewSpec.js Outdated Show resolved Hide resolved
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Fixed :D

Almost :-P I will send a final fix.

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2018

Failure unrelated: sh: 1: kill: No such process https://drone.nextcloud.com/nextcloud/server/12248/283

@skjnldsv
Copy link
Member

skjnldsv commented Nov 1, 2018

Almost :-P I will send a final fix.

Ahaha! Okay :)
Thanks a lot for your help @danxuliu 🤗

skjnldsv and others added 10 commits November 1, 2018 21:29
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Although now it is possible to create several link shares the acceptance
tests currently handles only the first link share; this first link share
is now created by clicking an "Add new share" button instead of a
checkbox.

Besides that, the "Copy link" button has been moved from the menu to the
row, next to the menu trigger.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@danxuliu danxuliu force-pushed the multiple-link-shares branch from 719e977 to 876d6ec Compare November 1, 2018 20:38
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Rejoice; finally, the nitpicker is reasonably satisfied 👍

I hope Drone is too... :-P

@MorrisJobke
Copy link
Member

🎉 I will merge once drone has done it's job 🎉

@lopezio
Copy link

lopezio commented May 3, 2019

Future interface stuff that is optional:

  • allow to give share links unique names (web ui)
  • allow to change unique name of a share (web ui)

@schiessle @jancborchardt any progress on these points..? I think they're fundamental to make the multiple linking functionality usable.. with some kind of distinction between the shared links it would be possible to explicitely delete or re-share links based on a target public / individual..

Best Regards!

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 feature: sharing high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple share links Share link not visible because of ad blockers
8 participants