1000772 - Desktop client needs to let a non signed-in user send a call-back URL by e-mail

Status: VERIFIED FIXED

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

Comment 1

[Romain Testard [:RT]]

This leverages mailto which uses the default e-mail client (hard client such as Thundebird or webclients such as GMail or Yahoo are supported as per about:preferences in the "applications" section).

Example:
mailto:?subject=Loop invitation to chat&body=Please click that link to call me back [LINK].

Comment 2

[Mark Banner (:standard8)]

Darrin, is "send by email" the default action of the share button? The MVP spec shows it as a list (with a default), but doesn't indicate what the items on the list are.

Comment 3

[Romain Testard [:RT]]

For MVP we only will allow the user to share by e-mail.
Beyond MVP there will be more options - Darrin maybe we remove the chevron as there is effectively just one option and use a different wording than "Share" to be more explicit about the fact that it will open your email composer?

Comment 4

[Darrin Henein [:darrin]]

That seems fair. The Share + chevron UI was intended to scale when we add more sharing options, though for now changing it to "Email" with no chevron seems reasonable until we actually have other options. Mark, is that clear enough? I will update the spec when I can.

Comment 5

[Romain Gauthier [:tOkeshu]]

Attachment: Add a button to send url by email

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8455304 [details] [diff] [review]
Add a button to send url by email

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

Can you update this to the latest master please? I think the patch for re-laying out the panel landed and this doesn't apply now.

Comment 7

[Romain Gauthier [:tOkeshu]]

Attachment: Add a button to send url by email

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8458700 [details] [diff] [review]
Add a button to send url by email

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

Looks good, but please add a test. This could be done by stubbing mozL10n.get to at least ensure that it's called with the right string id and call url.

::: browser/components/loop/content/js/panel.jsx
@@ +183,5 @@
>      render: function() {
> +      var mailto = [
> +        "mailto:?subject=" + __("share_email_subject") + "&",
> +        "body=" + __("share_email_body", {callUrl: this.state.callUrl})
> +      ].join("");

Could we move this to some "private" function, eg. _generateMailto?

Comment 9

[Romain Gauthier [:tOkeshu]]

Attachment: Add a button to send url by email

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8459591 [details] [diff] [review]
Add a button to send url by email

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

Looks good overall, minor nits to be optionally addressed; slightly more concerned with l10n and the non-urlencoded mailto link. r+ conditional to this not being an issue.

::: browser/components/loop/content/js/panel.jsx
@@ +183,5 @@
> +    _generateMailto: function() {
> +      return [
> +        "mailto:?subject=" + __("share_email_subject") + "&",
> +        "body=" + __("share_email_body", {callUrl: this.state.callUrl})
> +      ].join("");

Shouldn't we urlencode this string? I'm suspecting possible issues with l10n other than en_US.

There's this dirty but helpful hack which relies on a temporary <a> element:

> var a = document.createElement('a');
> a.href = "http://foo?été";
> a.href; // "http://foo/?%C3%A9t%C3%A9"

Not sure we really want to use this though; your take ;)

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +50,5 @@
> +          text = "{{callUrl}}";
> +        else
> +          text = "fakeText";
> +
> +        return JSON.stringify({textContent: text});

Nit: I'd rather see these defined contextually for each test.

@@ +311,5 @@
> +        var view = TestUtils.renderIntoDocument(loop.panel.CallUrlResult({
> +          notifier: notifier,
> +          client: fakeClient
> +        }));
> +        var shareButton;

Nit: unnecessary assignment.

@@ +316,5 @@
> +        view.setState({pending: false, callUrl: "http://example.com"});
> +
> +        TestUtils.findRenderedDOMComponentWithTag(view, "a");
> +        var shareButton = view.getDOMNode().querySelector("a.btn");
> +        expect(shareButton.href).to.equal(mailto);

See previous comment about urlencoding this string.

Comment 11

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Nicolas Perriault (:NiKo`) from comment #10)
> There's this dirty but helpful hack which relies on a temporary <a> element:
> 
> > var a = document.createElement('a');
> > a.href = "http://foo?été";
> > a.href; // "http://foo/?%C3%A9t%C3%A9"
> 
> Not sure we really want to use this though; your take ;)

You'll want to use encodeURIComponent(), obviously -_-'

Comment 12

[Romain Gauthier [:tOkeshu]]

Attachment: Add a button to send url by email

Comment 13

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8459655 [details] [diff] [review]
Add a button to send url by email

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

LGTM, r+. Please add a screenshot and submit it for UX review to :darrin, thanks! :)

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Actually, :sevaan is helping us out with UX review while :darrin is out of the office.

Comment 15

[Romain Gauthier [:tOkeshu]]

Attachment: share-email-button.png

Comment 16

[Sevaan Franks [:sevaan]]

Comment on attachment 8460175 [details]
share-email-button.png

Hi guys,

Just a couple questions (forgive my ignorance, as I'm just stepping in while Darrin is away)...

1) Is there a reason the button is green (#5cb85c) while in the spec it's blue (#0095dd)?
2) The button in the spec is 3px taller than the one in the screenshot.
3) Can we extend the width of the button so it takes up half the door hanger, again matching the spec?
4) The text in the button should also be bold.

Thanks,
Sevaan

Comment 17

[Maire Reavy [:mreavy]]

Hi Romain -- Can you answer and/or address Sevaan's questions in Comment 16?  Is this blocked only on UI-review?  This is one of the bugs that Chad and RT would like to get uplifted into Aurora.  Thanks.

Comment 18

[Darrin Henein [:darrin]]

(In reply to Sevaan Franks [:sevaan] from comment #16)
> Comment on attachment 8460175 [details]
> share-email-button.png

Thanks Sevaan, those comments are valid and should be resolved before ui r+.

Can we also have Matej look at the "Please click that link to call me back:" string? There may be room for improvement.

Comment 19

[Romain Gauthier [:tOkeshu]]

Attachment: Add a button to send url by email

Here is the last version of the patch. It should address UX comments.

Comment 20

[Romain Gauthier [:tOkeshu]]

Attachment: share-email-button.png

Comment 21

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8465562 [details] [diff] [review]
Add a button to send url by email

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

Looks good, some comments need to be addressed though.

::: browser/components/loop/content/js/panel.jsx
@@ +211,5 @@
>          <PanelLayout summary={__("share_link_header_text")}>
>            <div className="invite">
>              <input type="url" value={this.state.callUrl} readOnly="true"
>                     className={cx({'pending': this.state.pending})} />
> +          <a className={this.state.callUrl ? 'btn' : 'btn hide'}

Nit: You could use a classSet here http://facebook.github.io/react/docs/class-name-manipulation.html

::: browser/components/loop/content/shared/css/common.css
@@ +57,5 @@
>    clear: both;
>  }
>  
>  .hide {
> +  display: none !important;

Please add a comment why this is needed.

::: browser/components/loop/content/shared/css/panel.css
@@ +82,5 @@
> +  width: 50%;
> +  height: 26px;
> +  text-align: center;
> +  font-weight: bold;
> +}

I wish we went with a style reusable elsewhere, though it's probably good enough for now. Up to you.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +247,5 @@
>  
>      describe("Rendering the component should generate a call URL", function() {
>  
> +      beforeEach(function() {
> +        document.mozL10n.initialize({getStrings: function(key) {

Nit: This would be a bit more legible formatted this way:

document.mozL10n.initialize({
  getStrings: function(key) {
    var…

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +40,5 @@
>    target="_blank" href="{{terms_of_use_url}}">Terms of Use</a> and <a \
>    href="{{privacy_notice_url}}">Privacy Notice</a>.
> +
> +share_email_subject=Loop invitation to chat
> +share_email_body=Please click that link to call me back:\r\n\r\n{{callUrl}}

Nit: You may want to repeat the LOCALIZATION NOTE here, though it feels overkill. Up to you.

Comment 22

[Darrin Henein [:darrin]]

Comment on attachment 8465563 [details]
share-email-button.png

- text should be vertically centered within the button
- button group (will eventually be Email and Copy) should have more padding above and below (14px I believe)
- button color is incorrect
- button text color is incorrect

I know you were using http://people.mozilla.org/~mmaslaney/loop/Loop-spec.png which is great, I just want these buttons to have the Action style (blue with white text, see bottom of that spec).

Comment 23

[Matej Novak [:matej]]

(In reply to Darrin Henein [:darrin] from comment #18)
> (In reply to Sevaan Franks [:sevaan] from comment #16)
> > Comment on attachment 8460175 [details]
> > share-email-button.png
> 
> Thanks Sevaan, those comments are valid and should be resolved before ui r+.
> 
> Can we also have Matej look at the "Please click that link to call me back:"
> string? There may be room for improvement.

Is this a one-time use link? Or can the person always use it to reach the other person?

Here are a few options that cover both use cases:

If you ever need to reach me, use this link: 

Need to call me back? Use this link:

Here's a link you can use to call me back:

Call me back at this link:

Use this link to call me back:

Comment 24

[Maire Reavy [:mreavy]]

Do you think we can file the recommendations in Comment 23 as a follow up bug?  I'd like to make this particular bug about implementing the current spec since we currently have no email button on the UI.  Thanks.

Comment 25

[Matej Novak [:matej]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #24)
> Do you think we can file the recommendations in Comment 23 as a follow up
> bug?  I'd like to make this particular bug about implementing the current
> spec since we currently have no email button on the UI.  Thanks.

It's all the same to me.

Darrin, if you file a new bug, just assign it to me and I'll add the options there.

Comment 26

[Darrin Henein [:darrin]]

Sure -- was just commenting as the screenshots for review included the copy in the generated email (which I believe gets passed from this Email button).

Comment 27

[Chad Weiner [:chadw]]

Matej, not sure workflows change in our new world order, but flagging Eric and Arcadio who have an interest in this copy.

(In reply to Matej Novak [:matej] from comment #25)
> (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #24)
> > Do you think we can file the recommendations in Comment 23 as a follow up
> > bug?  I'd like to make this particular bug about implementing the current
> > spec since we currently have no email button on the UI.  Thanks.
> 
> It's all the same to me.
> 
> Darrin, if you file a new bug, just assign it to me and I'll add the options
> there.

Comment 28

[Maire Reavy [:mreavy]]

Hi Romain -- Can you put the new patch up to review to Niko (or one of the other Loop reviewers)?  I notice this has a feedback+, not an r+, and it's for the old patch.

Comment 29

[Romain Gauthier [:tOkeshu]]

Attachment: Add a button to send url by email

Comment 30

[Romain Gauthier [:tOkeshu]]

Attachment: share-email-button.png

Comment 31

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8468358 [details] [diff] [review]
Add a button to send url by email

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

LGTM

Comment 32

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8468358 [details] [diff] [review]
Add a button to send url by email

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

Sorry I need to cancel r+, because the jsx files have been compiled with react-tools v0.10.0, while we upgraded to 0.11.1 lately. Please update the patch to use the latest version, so we avoid unnecessary diffs.

Comment 33

[Romain Gauthier [:tOkeshu]]

Attachment: Add a button to send url by email

Comment 34

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8468421 [details] [diff] [review]
Add a button to send url by email

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

Looks good, thanks!

Comment 35

[Mark Banner (:standard8)]

Landed with the existing text:

https://hg.mozilla.org/integration/fx-team/rev/2e89d2bb6fac

If there's any follow-ups for change of text, please file them as new bugs.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e89d2bb6fac

Comment 37

[Francesco Lodolo [:flod]]

Before filing a new bug, I'd like to know why we're shooting ourselves in the foot by hard-coding "Loop" in the strings. That's the first one I've seen so far, so I'm asking, it might be on purpose.

If you want to expose the product name, and we should all know that "Loop" is not it, please put the name in branding and use a replacement in strings.

Comment 38

[Mark Banner (:standard8)]

(In reply to Francesco Lodolo [:flod] from comment #37)
> Before filing a new bug, I'd like to know why we're shooting ourselves in
> the foot by hard-coding "Loop" in the strings. That's the first one I've
> seen so far, so I'm asking, it might be on purpose.

Yeah, that's a mistake, I'm fixing it in bug 1050263.

Comment 39

[Romain Testard [:RT]]

Attachment: Yahoo mail

I have Yahoo Mail set to handle mailto in about:preferences and when I hit the button to share by e-mail rather than opening a new tab, Yahoo mail composer opens in the panel UI.

Comment 40

[Paul Silaghi, QA [:pauly]]

Verified fixed 34.0a1 (2014-09-02) Win 7, OS X 10.9.4, Ubuntu 13.04

Comment 41

[Mark Banner (:standard8)]

Removing apparently obsolete NI requests which were about the text for sending emails. I believe this has been overtaken by events, if any changes/additional discussion is required, please file a new bug so it can be tracked there.