Closed
Bug 1408334
Opened 7 years ago
Closed 7 years ago
[dt-onboarding] Add form to subscribe to devtools newsletter
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(firefox58 fixed)
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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
Assignee | ||
Updated•7 years ago
|
Blocks: dt-onboarding
Assignee | ||
Updated•7 years ago
|
No longer blocks: dt-addon-uishim
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?
No longer blocks: 1408339, dt-onboarding
Flags: needinfo?(victoria)
Summary: [dt-onboarding] Add form to subscribe to devtools mailing list → [dt-onboarding] Add form to subscribe to devtools newsletter
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
Flags: needinfo?(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
Assignee | ||
Updated•7 years ago
|
Blocks: 1408339, dt-onboarding
Comment 4•7 years ago
|
||
Mozilla's Developer Newsletter: https://www.mozilla.org/en-US/newsletter/developer/
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?
Flags: needinfo?(alainez)
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.
(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
Flags: needinfo?(alainez)
(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.
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?
Flags: needinfo?(alainez)
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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)
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?
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.
Attachment #8924633 -
Flags: review?(nchevobbe) → review-
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.
Comment hidden (mozreview-request) |
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
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 ?
Attachment #8924633 -
Flags: review?(nchevobbe) → review+
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.
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
Comment hidden (mozreview-request) |
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.
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
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?
Flags: needinfo?(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 "
Flags: needinfo?(jdescottes)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d417b26eb5120564413dc057ac9fa1aa398e23a2
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
Comment 31•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd50caefe127
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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?
Flags: needinfo?(alainez)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•