Closed Bug 1000771 Opened 6 years ago Closed 5 years ago

Desktop client needs to let non signed-in user copy a call-back URL

Categories

(Hello (Loop) :: Client, defect, P2)

defect

Tracking

(firefox34 verified)

VERIFIED FIXED
mozilla34
Tracking Status
firefox34 --- verified

People

(Reporter: RT, Unassigned)

References

()

Details

(Whiteboard: [p=2])

User Story

As a non signed-in FF browser user I can copy a call-back URL to the clipboard so that I can paste it on my preferred communication channel with someone.

Attachments

(1 file, 3 obsolete files)

No description provided.
User Story: (updated)
Priority: -- → P4
Whiteboard: [s=ui32]
Target Milestone: --- → mozilla32
No longer blocks: 1000770
Depends on: 1000770
Blocks: 972014
Priority: P4 → P1
Whiteboard: [s=ui32] → p=?
Target Milestone: mozilla32 → mozilla33
Blocks: 1014575
No longer blocks: 972014
Summary: Desktop client needs UI to let non signed-in user copy a call-back URL → Desktop client needs to let non signed-in user copy a call-back URL
Depends on: 1015891
Depends on: 1015941
Depends on: 1015944
you can do this with normal copy & paste from OS today, but the new UI has a button in Loop to do the copy to the system clipboard (pre-called, not signed in).  this may be another bug that requires a service on the API, so slightly more complicated than it looks.
Priority: P1 → P2
Whiteboard: p=? → p=2
Priority: P2 → P1
Whiteboard: p=2 → [p=2]
Target Milestone: mozilla33 → mozilla34
can do today using computer copy and paste and our first release users should know how to do this.  pushed to later when we have broader user adoption.
Target Milestone: mozilla34 → mozilla35
I agree although this will break the balance of the UX.
I NeedInfo Darrin for confirmation once he gets back.
Flags: needinfo?(dhenein)
My only concern is that users will not immediately know to select + copy that link. The copy button actually serves two purposes: to actually copy the URL, but also to cue the user that copy/paste is the primary method to transmit the link right now.

If it is indeed a lot of work, it can be pushed, but it is a critical part of the usage path and may affect success rates.
Flags: needinfo?(dhenein)
Thanks Darrin, good point and I agree we should include this in FF34.
Maire, it looks to be a p=2 delivering good user value - I'm marking it as FF34 P2 - we can discuss on our weekly call tomorrow.
Priority: P1 → P2
Target Milestone: mozilla35 → mozilla34
The shorter URL (mockup: http://talk.mozilla.org/Jc70K6OH vs nightly: https://call.mozilla.com/#call/0btpBYHGUYw) - is it handled somewhere in some bug?
Flags: needinfo?(rtestard)
Same question for the lock next to the URL in the mockup
(In reply to Paul Silaghi, QA [:pauly] from comment #6)
> The shorter URL (mockup: http://talk.mozilla.org/Jc70K6OH vs nightly:
> https://call.mozilla.com/#call/0btpBYHGUYw) - is it handled somewhere in
> some bug?

It was not, thanks for raising!
I have now created https://bugzilla.mozilla.org/show_bug.cgi?id=1046114 to track the implementation of the final URL format we'll want for Beta launch.
Until beta I suggest we keep https://call.mozilla.com/#call/<token> 
I needInfo Arcadio as he suggested we may want to use https://webrtc.firefox.com/<token> although I'm not sure it makes sense to implement that change given it won't be valid for a long period of time.
Flags: needinfo?(rtestard) → needinfo?(alainez)
So for some reason I'm having a hard time to track, the clipboard API doesn't seem to work in Firefox as it's supposed to (see http://caniuse.com/clipboard).

Here's a minimal example reproducing the issue: http://jsbin.com/volej/7/edit

I suggest we go with relying on exposing that feature through the mozLoop API for now. And if so, that's probably something a gecko developer wants to take.

NI mreavy for decision making here.
Flags: needinfo?(mreavy)
Niko, based on our conversation today after the standup, are you going to pair with someone on this?  (Feel free to still say, "no thanks".)
Flags: needinfo?(mreavy)
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #10)
> Niko, based on our conversation today after the standup, are you going to
> pair with someone on this?  (Feel free to still say, "no thanks".)

Mike sent me a pastebin (https://pastebin.mozilla.org/5840382) to get started, I should be all fine but will ask for help if needed :)
Attached patch Copy Loop call URL. (obsolete) — Splinter Review
This patch adds a new "Copy" button in the panel for when a call URL has been generated. It also refactors a little the way the "Email" button was handled, and related tests.

Following a discussion we had with Mike on IRC, the mozLoop.getString method isn't tested, as the gecko clipboard API is already, and we don't do much more than proxying it through the mozLoop object.
Attachment #8469207 - Flags: review?(mdeboer)
Comment on attachment 8469207 [details] [diff] [review]
Copy Loop call URL.

Review of attachment 8469207 [details] [diff] [review]:
-----------------------------------------------------------------

The approach looks solid, so we're past the feedback stage (would be f+), so I'm going with r-.

::: browser/components/loop/MozLoopAPI.jsm
@@ +227,5 @@
>          });
>        }
>      },
> +
> +    copyString: {

This needs a nice doc-block comment explaining what this function does, like the other API methods.

::: browser/components/loop/content/js/panel.jsx
@@ +263,5 @@
>    var PanelView = React.createClass({
>      propTypes: {
>        notifier: React.PropTypes.object.isRequired,
> +      client: React.PropTypes.object.isRequired,
> +      callUrl: React.PropTypes.string // Mostly used for UI components showcase

nit: put the comment above the addressed line and add 'and unit tests' to it.

::: browser/components/loop/content/shared/css/panel.css
@@ +102,5 @@
> +}
> +
> +.share .action .url-actions .btn:first-child {
> +  margin-right: 1em;
> +}

This will become fun when we start supporting RTL mode. Why not not `-moz-margin-end: 1em` on both buttons?

Also, please try to use child-selectors here.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +320,5 @@
> +              .to.equal(encodeURI(mailto));
> +      });
> +
> +      it("should feature a copy button capable of copying the call url when " +
> +         "clicked", function() {

nit: I don't see value in splitting this string across multiple lines.
Attachment #8469207 - Flags: review?(mdeboer) → review-
Attached patch Copy Loop call URL. (obsolete) — Splinter Review
Adressed review comments.
Attachment #8469207 - Attachment is obsolete: true
Attachment #8469249 - Flags: review?(mdeboer)
Attached patch Copy Loop call URL. (obsolete) — Splinter Review
Reverted to use -moz-margin-end as discussed with Mike on IRC.
Attachment #8469249 - Attachment is obsolete: true
Attachment #8469249 - Flags: review?(mdeboer)
Attachment #8469257 - Flags: review?(mdeboer)
Comment on attachment 8469257 [details] [diff] [review]
Copy Loop call URL.

Review of attachment 8469257 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks!!

::: browser/components/loop/MozLoopAPI.jsm
@@ +228,5 @@
>        }
>      },
> +
> +    /**
> +     * Copies passed string into end user's clipboard.

nit: can you change this to 'onto the system clipboard' ?
Attachment #8469257 - Flags: review?(mdeboer) → review+
Fixed latest nits.
Attachment #8469257 - Attachment is obsolete: true
Attachment #8469267 - Flags: review?(mdeboer)
Comment on attachment 8469267 [details] [diff] [review]
Copy Loop call URL.

Per irc, we agreed to carry forward the r=mikedeboer
Attachment #8469267 - Flags: review?(mdeboer) → review+
(In reply to Romain Testard [:RT - on PTO back on August 14th] from comment #8)
> It was not, thanks for raising!
> I have now created https://bugzilla.mozilla.org/show_bug.cgi?id=1046114 to
> track the implementation of the final URL format we'll want for Beta launch.
> Until beta I suggest we keep https://call.mozilla.com/#call/<token> 
> I needInfo Arcadio as he suggested we may want to use
> https://webrtc.firefox.com/<token> although I'm not sure it makes sense to
> implement that change given it won't be valid for a long period of time.

This is already dealt with in other bugs, clearing the needinfo request.
Flags: needinfo?(alainez)
https://hg.mozilla.org/mozilla-central/rev/b3528aeaf773
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Verified fixed in the latest Nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
You need to log in before you can comment on or make changes to this bug.