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

[9.2] Add clipboard button to public share link #25418

Merged
merged 2 commits into from Jul 15, 2016

Conversation

Projects
None yet
7 participants
@DeepDiver1975
Member

DeepDiver1975 commented Jul 8, 2016

bildschirmfoto von 2016-07-08 14-30-50

and I also moved the mail send button:
bildschirmfoto von 2016-07-08 15-22-52

@DeepDiver1975 DeepDiver1975 added this to the 9.2 milestone Jul 8, 2016

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jul 8, 2016

@DeepDiver1975, thanks for your PR! By analyzing the annotation information on this pull request, we identified @PVince81, @jancborchardt and @MorrisJobke to be potential reviewers

@DeepDiver1975, thanks for your PR! By analyzing the annotation information on this pull request, we identified @PVince81, @jancborchardt and @MorrisJobke to be potential reviewers

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG Jul 8, 2016

Member

screen shot 2016-07-08 at 14 51 10

Icon appears before sharing by link. It copies to clipboard "use-clipboard-api".

Member

SergioBertolinSG commented Jul 8, 2016

screen shot 2016-07-08 at 14 51 10

Icon appears before sharing by link. It copies to clipboard "use-clipboard-api".

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG Jul 8, 2016

Member

In safari, the copied string is the path of the file, not the public shared link.

Something like host/index.php/apps/files/?dir=/COSAS/SUBCOSAS&fileid=10
Instead of host/index.php/s/0wmlnsYFkKh5fpT

Member

SergioBertolinSG commented Jul 8, 2016

In safari, the copied string is the path of the file, not the public shared link.

Something like host/index.php/apps/files/?dir=/COSAS/SUBCOSAS&fileid=10
Instead of host/index.php/s/0wmlnsYFkKh5fpT

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jul 8, 2016

Member

Icon appears before sharing by link. It copies to clipboard "use-clipboard-api"

fixed

Member

DeepDiver1975 commented Jul 8, 2016

Icon appears before sharing by link. It copies to clipboard "use-clipboard-api"

fixed

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jul 8, 2016

Member

In safari, the copied string is the path of the file, not the public shared link.

Something like host/index.php/apps/files/?dir=/COSAS/SUBCOSAS&fileid=10
Instead of host/index.php/s/0wmlnsYFkKh5fpT

strange 💩

Member

DeepDiver1975 commented Jul 8, 2016

In safari, the copied string is the path of the file, not the public shared link.

Something like host/index.php/apps/files/?dir=/COSAS/SUBCOSAS&fileid=10
Instead of host/index.php/s/0wmlnsYFkKh5fpT

strange 💩

@jancborchardt

This comment has been minimized.

Show comment
Hide comment
@jancborchardt

jancborchardt Jul 8, 2016

Member

Ideally the link should directly be copied to the clipboard as soon as you click »Share link«. That will make the flow even more seamless. Can you change it so that happens automatically?

Member

jancborchardt commented Jul 8, 2016

Ideally the link should directly be copied to the clipboard as soon as you click »Share link«. That will make the flow even more seamless. Can you change it so that happens automatically?

@jvillafanez

This comment has been minimized.

Show comment
Hide comment
@jvillafanez

jvillafanez Jul 11, 2016

Member

@jancborchardt I haven't seen that behaviour in any other app. It seems unnatural that clicking in the checkbox copies the text. What if the user unchecks it later? What would happen with the clipboard?

In addition, users won't know that the link is copied unless they're told explicitly. If we don't use any kind of notification, this feature won't be used properly. So, where and how will the notification be placed in this case?

Moreover, the previous clipboard contents will be overwritten. I'd rather have an explicit user interaction. We don't know what will the user do with the link, and we shouldn't suppose that he will want to copy the link.

Member

jvillafanez commented Jul 11, 2016

@jancborchardt I haven't seen that behaviour in any other app. It seems unnatural that clicking in the checkbox copies the text. What if the user unchecks it later? What would happen with the clipboard?

In addition, users won't know that the link is copied unless they're told explicitly. If we don't use any kind of notification, this feature won't be used properly. So, where and how will the notification be placed in this case?

Moreover, the previous clipboard contents will be overwritten. I'd rather have an explicit user interaction. We don't know what will the user do with the link, and we shouldn't suppose that he will want to copy the link.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jul 11, 2016

Member

@jancborchardt I haven't seen that behaviour in any other app. It seems unnatural that clicking in the checkbox copies the text. What if the user unchecks it later? What would happen with the clipboard?

In addition, users won't know that the link is copied unless they're told explicitly. If we don't use any kind of notification, this feature won't be used properly. So, where and how will the notification be placed in this case?

Moreover, the previous clipboard contents will be overwritten. I'd rather have an explicit user interaction. We don't know what will the user do with the link, and we shouldn't suppose that he will want to copy the link.

Notifications can be implemented using the tooltip - this should not be an issue.

But I agree on the matter that the clipboard is overwritten which is somewhat unexpected and make the user angry if some valuable content is in there.

Member

DeepDiver1975 commented Jul 11, 2016

@jancborchardt I haven't seen that behaviour in any other app. It seems unnatural that clicking in the checkbox copies the text. What if the user unchecks it later? What would happen with the clipboard?

In addition, users won't know that the link is copied unless they're told explicitly. If we don't use any kind of notification, this feature won't be used properly. So, where and how will the notification be placed in this case?

Moreover, the previous clipboard contents will be overwritten. I'd rather have an explicit user interaction. We don't know what will the user do with the link, and we shouldn't suppose that he will want to copy the link.

Notifications can be implemented using the tooltip - this should not be an issue.

But I agree on the matter that the clipboard is overwritten which is somewhat unexpected and make the user angry if some valuable content is in there.

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Jul 11, 2016

Member

we shouldn't suppose that he will want to copy the link.

But creating a link without copying it is kind of senseless, isn't it? I never created a link share (beside testing of the sharing app) and didn't copied it.

Member

MorrisJobke commented Jul 11, 2016

we shouldn't suppose that he will want to copy the link.

But creating a link without copying it is kind of senseless, isn't it? I never created a link share (beside testing of the sharing app) and didn't copied it.

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Jul 11, 2016

Member

But creating a link without copying it is kind of senseless, isn't it? I never created a link share (beside testing of the sharing app) and didn't copied it.

I just have no thing in mind: sending it via email: but then there should be definitely something like a "guide":

you created a share: What to do with this share? Send via email - copy link - ...

Member

MorrisJobke commented Jul 11, 2016

But creating a link without copying it is kind of senseless, isn't it? I never created a link share (beside testing of the sharing app) and didn't copied it.

I just have no thing in mind: sending it via email: but then there should be definitely something like a "guide":

you created a share: What to do with this share? Send via email - copy link - ...

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jul 11, 2016

Member

I just have no thing in mind: sending it via email: but then there should be definitely something like a "guide":

you created a share: What to do with this share? Send via email - copy link - ...

There are for sure things which can be improved - let's for now just introduce the copy link button.
There is nothing really playing against this - right?

Member

DeepDiver1975 commented Jul 11, 2016

I just have no thing in mind: sending it via email: but then there should be definitely something like a "guide":

you created a share: What to do with this share? Send via email - copy link - ...

There are for sure things which can be improved - let's for now just introduce the copy link button.
There is nothing really playing against this - right?

@jancborchardt

This comment has been minimized.

Show comment
Hide comment
@jancborchardt

jancborchardt Jul 11, 2016

Member

Yeah, let’s go step-by-step. I still think automatically copying it makes the flow even easier and reduces a step you are surely gonna take, which is copying the link.

The clipboard button we need in any case for when you want to get an existing shared link quickly, so 👍

Member

jancborchardt commented Jul 11, 2016

Yeah, let’s go step-by-step. I still think automatically copying it makes the flow even easier and reduces a step you are surely gonna take, which is copying the link.

The clipboard button we need in any case for when you want to get an existing shared link quickly, so 👍

DeepDiver1975 added some commits Jul 8, 2016

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jul 15, 2016

Member

@SergioBertolinSG safari has been fixed as far as technically possible.

This is the way this works once the user with safari hits the button:

  • the text in the input field is selected
  • a tool tip will popup telling the user to press cmd+C to copy

In addition on ipad and ihone the tooltiop will tell the user that it is not supported - maybe better in this case would be to simply not show the button - but this is a future improvment from my pov

Member

DeepDiver1975 commented Jul 15, 2016

@SergioBertolinSG safari has been fixed as far as technically possible.

This is the way this works once the user with safari hits the button:

  • the text in the input field is selected
  • a tool tip will popup telling the user to press cmd+C to copy

In addition on ipad and ihone the tooltiop will tell the user that it is not supported - maybe better in this case would be to simply not show the button - but this is a future improvment from my pov

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG Jul 15, 2016

Member

Works fine 👍

Tested on chrome, safari (with that workaround) and edge.

Member

SergioBertolinSG commented Jul 15, 2016

Works fine 👍

Tested on chrome, safari (with that workaround) and edge.

clipboard.on('error', function (e) {
$input = $(e.trigger);
var actionMsg = '';
if (/iPhone|iPad/i.test(navigator.userAgent)) {

This comment has been minimized.

@PVince81

PVince81 Jul 15, 2016

Member

if we ever want to reuse that button elsewhere, would be good to have all this logic in a central place, like js.js in OC.Util or so

@PVince81

PVince81 Jul 15, 2016

Member

if we ever want to reuse that button elsewhere, would be good to have all this logic in a central place, like js.js in OC.Util or so

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Jul 15, 2016

Member

We can do this once necessary - I have no clue what the perf impact is if we setup the clipboard instance in js.js

@DeepDiver1975

DeepDiver1975 Jul 15, 2016

Member

We can do this once necessary - I have no clue what the perf impact is if we setup the clipboard instance in js.js

This comment has been minimized.

@PVince81

PVince81 Jul 15, 2016

Member

Agreed. Note that the way I see it is more like a method OC.Util.setupClipboard($inputField) that does this lazily.

Anyway, let's merge this for now.

@PVince81

PVince81 Jul 15, 2016

Member

Agreed. Note that the way I see it is more like a method OC.Util.setupClipboard($inputField) that does this lazily.

Anyway, let's merge this for now.

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Jul 15, 2016

Member

Anyway, let's merge this for now.

THX 🎉

@DeepDiver1975

DeepDiver1975 Jul 15, 2016

Member

Anyway, let's merge this for now.

THX 🎉

@PVince81 PVince81 merged commit d4c0838 into master Jul 15, 2016

11 of 12 checks passed

core-ci-linux-swift-primary-storage/database=mysql,label=SLAVE Build #57553 failed in 40 sec
Details
Scrutinizer 9 new issues
Details
cla-bot-core Build #5341 succeeded in 9 sec
Details
continuous-integration/php-5.4 Build #5917 succeeded in 20 min
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #63663 succeeded in 9 min 56 sec
Details
core-ci-linux/database=mysql,label=SLAVE Build #32503 succeeded in 24 min
Details
core-ci-linux/database=oci,label=SLAVE Build #32503 succeeded in 51 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #32503 succeeded in 45 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #32503 succeeded in 9 min 21 sec
Details
ocs-api-integration-tests-ci Build #12258 succeeded in 15 min
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #40893 succeeded in 15 min
Details

@PVince81 PVince81 deleted the use-clipboard-api branch Jul 15, 2016

@LukasReschke LukasReschke referenced this pull request Jul 20, 2016

Merged

Use clipboard api #467

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 20, 2016

Great, guys! For my Passwords app I really would like to have it in a global function.
For now, I'll delete my own dependency and copy your code. Thanks again!

ghost commented Sep 20, 2016

Great, guys! For my Passwords app I really would like to have it in a global function.
For now, I'll delete my own dependency and copy your code. Thanks again!

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Sep 20, 2016

Member

@fcturner please have a look at #25757 - it ships the global function - let me know if this works out for you - THX

Member

DeepDiver1975 commented Sep 20, 2016

@fcturner please have a look at #25757 - it ships the global function - let me know if this works out for you - THX

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 20, 2016

That's awesome! I'll let you know, thanks!

ghost commented Sep 20, 2016

That's awesome! I'll let you know, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment