Julian Descottes [:jdescottes] Assignee
	Description • 7 years ago

We are landing a first version of the devtools onboarding page in bug 1361080.

The installation complete page should contain a form to subscribe to the dev-developer-tools mailing list. [1]

See mockups at https://docs.google.com/document/d/1ku6Jn12XdTFNno0phhLjlBD2H43k5NYswALYnA_UVBA/edit#heading=h.vihrfx1f6nm2

[1] https://lists.mozilla.org/listinfo/dev-developer-tools

	Julian Descottes [:jdescottes] Assignee
	Comment 1 • 7 years ago

Victoria: looking at the mockups, this is not about subscribing to the devtools mailing list (just as I was thinking to myself that this was not really interesting for most DevTools users), but for the newsletter. 

Since the newsletter is not there yet, I am removing this bug from blocking the rest of the devtools onboarding work. We should revisit once the newsletter is created.

Do you think we should add some other content to the "enabling complete" page in the meantime?

	Victoria Wang [:victoria]
	Comment 2 • 7 years ago

Hi Julian, it's actually for the Mozilla Developer newsletter (same as the one at the bottom of https://www.mozilla.org/en-US/firefox/developer/). Not sure if this helps, but Marketing sent this link for the form's implementation: https://github.com/mozilla/basket-example

	Victoria Wang [:victoria]
	Comment 3 • 7 years ago

Also! In case it makes it easier to do this newsletter part, I finally got set up with Zeplin so you can inspect the mockup: https://zpl.io/aNwWvA9

Exact styling for the field should be based on this, if possible: http://design.firefox.com/photon/components/input-fields.html

	:Harald Kirschner :digitarald
	Comment 4 • 7 years ago

Mozilla's Developer Newsletter: https://www.mozilla.org/en-US/newsletter/developer/

	:Harald Kirschner :digitarald
	Comment 5 • 7 years ago

Arcadio, should we track sign ups with telemetry or is there a way to tag sign ups so we can capture a completion rate?

	:Harald Kirschner :digitarald
	Comment 6 • 7 years ago

Arcadio, do we have numbers on HTML/Text usage on the developer newsletter? We are thinking about defaulting to HTML here but want to confirm that this is the main preference.

	alainez
	Comment 7 • 7 years ago

(In reply to :Harald Kirschner :digitarald from comment #5)
> Arcadio, should we track sign ups with telemetry or is there a way to tag
> sign ups so we can capture a completion rate?

Through the newsletter we can read conversion rates (people who sign up for the email newsletter). I don't think you need telemetry for this

	alainez
	Comment 8 • 7 years ago

(In reply to :Harald Kirschner :digitarald from comment #6)
> Arcadio, do we have numbers on HTML/Text usage on the developer newsletter?
> We are thinking about defaulting to HTML here but want to confirm that this
> is the main preference.

We started the newsletter by providing HTML and TEXT options. Initially, each send was about 90% HTML/10% TEXT. It looks like we've been sending HTML exclusively since April 2017 with a very consistent open rate. I would use HTML as default.

	:Harald Kirschner :digitarald
	Comment 9 • 7 years ago

> Through the newsletter we can read conversion rates (people who sign up for the email newsletter). I don't think you need telemetry for this

Is there something like UTM campaigns for newsletter sign ups so you can tell that users signed up from the onboarding page?

	Julian Descottes [:jdescottes] Assignee
	Comment 14 • 7 years ago

Nicolas: some additional information for the review:
- you can see the newsletter subscribe form in about:devtools (devtools.enabled needs to be true)
- you can use success@example.com and failure@example.com in order to test the success/error cases without providing an actual email address
- the action URL might change depending on the outcome of Bug 1413953 (right now I can't pass the source_url parameter pointing to about:devtools, but on the other hand the current URL supports the example email addresses, so I believe it's better to keep this one for testing)

	Julian Descottes [:jdescottes] Assignee
	Comment 15 • 7 years ago

(In reply to alainez from comment #8)
> (In reply to :Harald Kirschner :digitarald from comment #6)
> > Arcadio, do we have numbers on HTML/Text usage on the developer newsletter?
> > We are thinking about defaulting to HTML here but want to confirm that this
> > is the main preference.
> 
> We started the newsletter by providing HTML and TEXT options. Initially,
> each send was about 90% HTML/10% TEXT. It looks like we've been sending HTML
> exclusively since April 2017 with a very consistent open rate. I would use
> HTML as default.

Thanks for the info Arcadio, we'll just use HTML for now.

(In reply to :Harald Kirschner :digitarald from comment #9)
> > Through the newsletter we can read conversion rates (people who sign up for the email newsletter). I don't think you need telemetry for this
> 
> Is there something like UTM campaigns for newsletter sign ups so you can
> tell that users signed up from the onboarding page?

There is actually a source_url parameter we can send when submitting a subscription. It is currently buggy for about:devtools (there is a patch in progress for bedrock, even if it doesn't make it in time I can switch to another URL, see Bug 1413953 for details). 

Not sure how this is stored/made available though?

	Nicolas Chevobbe [:nchevobbe]
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8924633 [details]
Bug 1408334 - add form to subscribe to mozilla developer newsletter in about:devtools;

https://reviewboard.mozilla.org/r/195856/#review202146

I have a few nits / questions on the patch.

::: devtools/shim/aboutdevtools/aboutdevtools.css:53
(Diff revision 4)
>  }
>  
>  .left-pane {
> -  width: 360px;
> -  height: 100%;
> +  width: 300px;
> +  height: 300px;
> +  margin-right: 20px;

do we really want `right` or could it be `margin-inline-end` ?

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:65
(Diff revision 4)
> +            <!-- "H" stands for the HTML format (->fmt). Alternative is T for text. -->
> +            <input type="hidden" id="fmt" name="fmt" value="H" />
> +            <!-- "app-dev" is the id of the Mozilla Developper newsletter -->
> +            <input type="hidden" id="newsletters" name="newsletters" value="app-dev" />
> +            <div id="newsletter-errors"></div>
> +            <div id="newsletter-email" class="newsletter-form-section">

this could be a <section> tag

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:66
(Diff revision 4)
> +            <input type="hidden" id="fmt" name="fmt" value="H" />
> +            <!-- "app-dev" is the id of the Mozilla Developper newsletter -->
> +            <input type="hidden" id="newsletters" name="newsletters" value="app-dev" />
> +            <div id="newsletter-errors"></div>
> +            <div id="newsletter-email" class="newsletter-form-section">
> +              <input type="email" id="email" name="email" required="true" placeholder="&aboutDevtools.newsletter.email.placeholder;" />

I know this is by design, but placeholders do not replace label (https://www.nngroup.com/articles/form-design-placeholders/)

I don't feel strongly about it, since the form is pretty simple here. Maybe we could file a follow up to think about it (not required too).

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:69
(Diff revision 4)
> +            <div id="newsletter-errors"></div>
> +            <div id="newsletter-email" class="newsletter-form-section">
> +              <input type="email" id="email" name="email" required="true" placeholder="&aboutDevtools.newsletter.email.placeholder;" />
> +            </div>
> +
> +            <div id="newsletter-privacy" class="newsletter-form-section">

this could be a <section> tag as well

::: devtools/shim/aboutdevtools/aboutdevtools.xhtml:73
(Diff revision 4)
> +            <div>
> +              <button type="submit" class="primary-button">&aboutDevtools.newsletter.subscribe.label;</button>
> +            </div>

do we need to wrap the button in a div ?

::: devtools/shim/aboutdevtools/subscribe.css:6
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * This file contains the styles the newsletter subscription form on about:devtools.

This file contains the styles the newsletter
->
This file contains the styles for the newsletter

::: devtools/shim/aboutdevtools/subscribe.css:54
(Diff revision 4)
> +
> +#newsletter-privacy {
> +  display: flex;
> +
> +  /* The privacy section is hidden by default and only displayed on focus */
> +  transition: all 0.5s;

According to http://design.firefox.com/photon/motion/duration-and-easing.html , this should be faster and use the provided curve

::: devtools/shim/aboutdevtools/subscribe.css:61
(Diff revision 4)
> +  /* Hardcoded target height for the transition that shows the privacy section */
> +  height: 65px;

can we do it without hardcoded value ? There might be locale (i.e. german) / screen sizes where the content will overflow

::: devtools/shim/aboutdevtools/subscribe.js:42
(Diff revision 4)
> +    for (let error of errors) {
> +      let item = document.createElement("p");
> +      item.classList.add("error");
> +      item.appendChild(document.createTextNode(error));
> +      newsletterErrors.appendChild(item);
> +    }

nothing too important here, but could we use a document fragment to put all the "p" items, and then append the fragment into the error section ? If we have multiple errors, we only do 1 operation on the dom

::: devtools/shim/aboutdevtools/subscribe.js:57
(Diff revision 4)
> +    while (newsletterErrors.firstChild) {
> +      newsletterErrors.firstChild.remove();
> +    }

out of curiosity, why don't we use `newsletterErrors.innerHTML = "";` here ?

::: devtools/shim/aboutdevtools/subscribe.js:75
(Diff revision 4)
> +    let fmt = document.getElementById("fmt").value;
> +    let email = document.getElementById("email").value;
> +    let newsletter = document.getElementById("newsletters").value;
> +    let isPrivacyChecked = document.getElementById("privacy").checked;
> +
> +    let params = "email=" + encodeURIComponent(email) +
> +                  "&newsletters=" + newsletter +
> +                  (isPrivacyChecked ? "&privacy=true" : "") +
> +                  "&fmt=" + fmt;

Could we use FormData + URLSearchParams for this ?

::: devtools/shim/aboutdevtools/subscribe.js:100
(Diff revision 4)
> +        } else {
> +          // We trust the error messages from the service to be meaningful for the user.
> +          updateErrorPanel(response.errors);
> +        }
> +      } else {
> +        updateErrorPanel();

could we pass in the status code and message, and display it (instead of the "unknown error" message we'll show at the moment)

::: devtools/shim/aboutdevtools/tmp-locale/aboutdevtools.dtd:29
(Diff revision 4)
>  <!ENTITY  aboutDevtools.welcome.title "Welcome to Firefox Developer Tools!">
>  <!ENTITY  aboutDevtools.welcome.message "You’ve successfully enabled DevTools! To get started, explore the Web Developer menu or open the tools with ##INSPECTOR_SHORTCUT##.">
> +
> +<!ENTITY  aboutDevtools.newsletter.title "Mozilla Developer Newsletter">
> +<!ENTITY  aboutDevtools.newsletter.message "Get developer news, tricks and resources sent straight to your inbox.">
> +<!ENTITY  aboutDevtools.newsletter.email.placeholder "Email">

I'd rather display a `email@example.com` placeholder and have a proper label. but this can be discussed later.

	Julian Descottes [:jdescottes] Assignee
	Comment 17 • 7 years ago
mozreview-review-reply

Comment on attachment 8924633 [details]
Bug 1408334 - add form to subscribe to mozilla developer newsletter in about:devtools;

https://reviewboard.mozilla.org/r/195856/#review202146

> do we really want `right` or could it be `margin-inline-end` ?

We probably want inline-end. I have been inconsistent with this, I'll need to do an RTL pass on the page.

> I know this is by design, but placeholders do not replace label (https://www.nngroup.com/articles/form-design-placeholders/)
> 
> I don't feel strongly about it, since the form is pretty simple here. Maybe we could file a follow up to think about it (not required too).

The current implementation follows the design mockups from Victoria, we can discuss them in a follow up.

> According to http://design.firefox.com/photon/motion/duration-and-easing.html , this should be faster and use the provided curve

Thanks for the pointer! 
I used 0.25s cubic-bezier(.15,.75,.35,.9) (so slightly smaller curve) because the original one felt too fast IMO

> can we do it without hardcoded value ? There might be locale (i.e. german) / screen sizes where the content will overflow

That's a good point, would be great to do it without a hard coded value. I only know how to do this with JS though, let me know if you have a CSS only approach in mind.

> nothing too important here, but could we use a document fragment to put all the "p" items, and then append the fragment into the error section ? If we have multiple errors, we only do 1 operation on the dom

Done.

> out of curiosity, why don't we use `newsletterErrors.innerHTML = "";` here ?

I can't think of a good reason, that was the initial implementation. I'll change it I really don't see what could be wrong here.

> Could we use FormData + URLSearchParams for this ?

Good point, done.

> could we pass in the status code and message, and display it (instead of the "unknown error" message we'll show at the moment)

Sure, I still added a message to wrap it. e.g. will display "Subscription request failed (404 - Not Found)."

> I'd rather display a `email@example.com` placeholder and have a proper label. but this can be discussed later.

Yes, let's discuss the mockups in a follow up.

	Nicolas Chevobbe [:nchevobbe]
	Comment 19 • 7 years ago
mozreview-review-reply

Comment on attachment 8924633 [details]
Bug 1408334 - add form to subscribe to mozilla developer newsletter in about:devtools;

https://reviewboard.mozilla.org/r/195856/#review202146

> That's a good point, would be great to do it without a hard coded value. I only know how to do this with JS though, let me know if you have a CSS only approach in mind.

I think we can do something like 

```
#newsletter-privacy {
  ...
  height: auto;
  max-height: 0;
  transition: max-height …
}

#newsletter-privacy.show {
  /* Use a large number so it will always be greater than the actual height.
   * We need for the privacy panel show transition.
   */
  max-height: 1000px;
}
```

if it does not work, let's keep the hardcoded value

	Nicolas Chevobbe [:nchevobbe]
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8924633 [details]
Bug 1408334 - add form to subscribe to mozilla developer newsletter in about:devtools;

https://reviewboard.mozilla.org/r/195856/#review202238

I have a couple of comment on the FormData use, but the patch looks good to me. 
Let's see if we can have the animation for the privacy panel be CSS only. If we can't, your solution seems good.

::: devtools/shim/aboutdevtools/subscribe.js:66
(Diff revision 5)
> +    let container = document.createElement("div");
> +    container.style.cssText = "visibility: hidden; overflow: hidden; position: absolute";
> +    newsletterPrivacySection.parentNode.appendChild(container);
> +
> +    // Clone the privacy section, append the clone to the measuring container.
> +    let clone = newsletterPrivacySection.cloneNode(true);
> +    container.appendChild(clone);
> +
> +    // Measure the target height of the privacy section.
> +    clone.style.height = "auto";
> +    let height = clone.offsetHeight;
> +
> +    // Cleanup the mearuring container.
> +    container.remove();
> +
> +    // Set the animate class and set the height to the measured height.
> +    newsletterPrivacySection.classList.add("animate");
> +    newsletterPrivacySection.style.cssText = `height: ${height}px; margin-bottom: 0;`;

let's see if we can use the CSS solution :)

::: devtools/shim/aboutdevtools/subscribe.js:78
(Diff revision 5)
> +
> +    // Measure the target height of the privacy section.
> +    clone.style.height = "auto";
> +    let height = clone.offsetHeight;
> +
> +    // Cleanup the mearuring container.

s/mearuring/measuring

::: devtools/shim/aboutdevtools/subscribe.js:135
(Diff revision 5)
> +    let formData = new FormData();
> +
> +    formData.append("email", document.getElementById("email").value);
> +    formData.append("newsletters", document.getElementById("newsletters").value);
> +    formData.append("fmt", document.getElementById("fmt").value);
> +    formData.append("source_url", document.location.href);
> +
> +    let isPrivacyChecked = document.getElementById("privacy").checked;
> +    if (isPrivacyChecked) {
> +      formData.append("privacy", true);
> +    }

I thought we could do : 
```
// Get form values
let formData = new FormData(newsletterForm);
// append source url
formData.append("source_url", document.location.href);
```

::: devtools/shim/aboutdevtools/subscribe.js:147
(Diff revision 5)
> +    let params = new URLSearchParams(formData);
> +
> +    // Send the request.
> +    xhr.send(params.toString());

After looking at https://developer.mozilla.org/en-US/docs/Web/API/FormData/Using_FormData_Objects#Retrieving_a_FormData_object_from_an_HTML_form , it seems we could do `xhr.send(formData)` directly.

Not sure if it encodes values as we want though.

::: devtools/shim/aboutdevtools/tmp-locale/aboutdevtools.dtd:28
(Diff revision 5)
>  <!ENTITY  aboutDevtools.enable.closeButton "Close this page">
>  <!ENTITY  aboutDevtools.welcome.title "Welcome to Firefox Developer Tools!">
>  <!ENTITY  aboutDevtools.welcome.message "You’ve successfully enabled DevTools! To get started, explore the Web Developer menu or open the tools with ##INSPECTOR_SHORTCUT##.">
> +
> +<!ENTITY  aboutDevtools.newsletter.title "Mozilla Developer Newsletter">
> +<!ENTITY  aboutDevtools.newsletter.message "Get developer news, tricks and resources sent straight to your inbox.">

Is there a way to tell translater that the newsletter will be only in English ?

	Julian Descottes [:jdescottes] Assignee
	Comment 21 • 7 years ago

The max-height trick is not ideal, because the transition duration/curve is calculated based on the max height difference. If we set a max-height of 1000px to be safe, but the actual needed height is 50px, the "final" height will be reached as soon as the transition reaches "max-height: 50px" which will be very early in the transition. Not to mention that it only respects "linear" transition functions. I tried it earlier and was not happy with the result. 

I'll keep the JS solution for now.

	Julian Descottes [:jdescottes] Assignee
	Comment 22 • 7 years ago
mozreview-review-reply

Comment on attachment 8924633 [details]
Bug 1408334 - add form to subscribe to mozilla developer newsletter in about:devtools;

https://reviewboard.mozilla.org/r/195856/#review202238

> I thought we could do : 
> ```
> // Get form values
> let formData = new FormData(newsletterForm);
> // append source url
> formData.append("source_url", document.location.href);
> ```

Yep sorry :)

> After looking at https://developer.mozilla.org/en-US/docs/Web/API/FormData/Using_FormData_Objects#Retrieving_a_FormData_object_from_an_HTML_form , it seems we could do `xhr.send(formData)` directly.
> 
> Not sure if it encodes values as we want though.

Looks like the server doesn't accept the encoding here, have to go through URLSearchParams.

> Is there a way to tell translater that the newsletter will be only in English ?

I'll mention it in Bug 1408369

	Julian Descottes [:jdescottes] Assignee
	Comment 24 • 7 years ago

Thanks for the reviews Nicolas! Try is green at https://treeherder.mozilla.org/#/jobs?repo=try&revision=798ce6ef23246d82fc1882e761ee4b3b483f8635, landing.

	Pulsebot
	Comment 25 • 7 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a8295d1ff40
add form to subscribe to mozilla developer newsletter in about:devtools;r=nchevobbe

	Eliza Balazs [:ebalazs_]
	Comment 26 • 7 years ago

Backed out changeset 3a8295d1ff40 (bug 1408334) for failing bc in /browser_misused_characters_in_strings.js r=backout on a CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3a8295d1ff40d1778e41a1fa1e72cda7692575e0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

https://hg.mozilla.org/integration/autoland/rev/95ab305666d4cc3759f93579c6c9cad8e82f3be8

:jdescottes, can you please take a look?

	Julian Descottes [:jdescottes] Assignee
	Comment 27 • 7 years ago

Issue is with:

<!ENTITY  aboutDevtools.newsletter.privacy.label "I'm okay with Mozilla handling my info as explained in this <a class='external' href='https://www.mozilla.org/privacy/'>Privacy Policy</a>.">

Initially the problem was the singlequote in "I'm", but the test still fails after that because of the <a> arguments which are in single quotes.

This string is replicating a string we can see on mozilla.org, and which seems to be similarly localized on pontoon:
https://pontoon.mozilla.org/en-GB/mozillaorg/main.lang/?search=privacy&string=149450

I will escape the argument quotes as &#x0022;

	Julian Descottes [:jdescottes] Assignee
	Comment 29 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d417b26eb5120564413dc057ac9fa1aa398e23a2

	Pulsebot
	Comment 30 • 7 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd50caefe127
add form to subscribe to mozilla developer newsletter in about:devtools;r=nchevobbe

	Narcis Beleuzu [:NarcisB]
	Comment 31 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dd50caefe127

	alainez
	Comment 32 • 7 years ago

(In reply to :Harald Kirschner :digitarald from comment #9)
> > Through the newsletter we can read conversion rates (people who sign up for the email newsletter). I don't think you need telemetry for this
> 
> Is there something like UTM campaigns for newsletter sign ups so you can
> tell that users signed up from the onboarding page?