Adds newsletter subscription form to footer (closes #1521). #1620

chuckharmston · 2016-10-15T07:31:03Z

Still needs tests, but I thought I'd get it out for @johngruen to look at visuals Monday morning.

Some implementation notes:

I built the CSS thinking about Consider using CSS modules #1363 (one file per component with a single class' namespace for everything therein).
I tried to abstract the form from the implementation of the form, so that we can use it in other places (i.e. post-Test Pilot install) with minimal effort.

TODO

Rename everything from email to newsletter.
Use the mergeProps argument to connect to namespace setEmailForm* to keep everything clean.
Don't try to plan for future forms, that's premature optimization.
Make subscribeToBasketdriven by a dispatched action, rather than a callback.
Write tests for all the things I introduced.

Testing

Using the redux dev tools extension, you can simulate error and success states without submitting the form by dispatching the following action payloads:

Submitting

{
  type: 'newsletterFormSetSubmitting'
}

Error

{
  type: 'newsletterFormSetFailed'
}

Success

{
  type: 'newsletterFormSetSucceeded'
}

Screenshots

Default

After entering a character

When trying to submit with an empty email

When trying to submit with an invalid email

When trying to submit without agreeing to the privacy notice

At medium width

At small width

On success

On error

chuckharmston · 2016-10-17T16:04:59Z

The button and input should maybe be shrunk down a bit at medium widths.

johngruen

@chuckharmston looks like the form is leaking into some extra pages:

johngruen · 2016-10-17T17:27:34Z

@chuckharmston also noticed it on the uninstall test pilot interstitial page

johngruen · 2016-10-17T17:27:59Z

Otherwise UX looks solid, i'll leave it to someone else to do a code review

johngruen · 2016-10-17T20:45:29Z

@chuckharmston one more nit... after the email, the string should be "We will only send you Test Pilot related information".

pdehaan · 2016-10-18T16:33:58Z

frontend/src/app/components/EmailFooter.js

+        <p data-l10n-id="emailFooterSuccessBody">
+          If you haven't previously confirmed a subscription to a Mozilla-related
+          newsletter you may have to do so. Please check your inbox or your spam
+          filter for an email from us.


You say "email" here, but you hyphenated ("e-mail") on line 27 above.

@chuckharmston @pdehaan let's stick to email

pdehaan · 2016-10-18T16:38:47Z

frontend/src/images/email.svg

+<svg width="146px" height="127px" viewBox="0 0 146 127" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <!-- Generator: Sketch 40.2 (33826) - http://www.bohemiancoding.com/sketch -->
+    <title>Email</title>
+    <desc>Created with Sketch.</desc>


Probably want to run this file through SVGO or something.

pdehaan · 2016-10-18T16:41:12Z

locales/en-US/app.ftl

+emailFormSubmitButton = Sign Up Now
+emailFormSubmitButtonSubmitting = Submitting...
+
+emailFooterError = There was an error submitting your e-mail address. Try again?


e-mail -> email (for consistency w/ line 167 and 177.

pdehaan · 2016-10-18T16:43:38Z

frontend/src/styles/components/EmailFooter.scss

+$email-icon--responsive-unit: .66;
+
+.email-footer {
+  background: $transparent-white-1;


Error in plugin 'sass' Message: frontend/src/styles/components/EmailFooter.scss Error: Undefined variable: "$transparent-white-1". on line 7 of frontend/src/styles/components/EmailFooter.scss >> background: $transparent-white-1; --------------^

@johngruen had similar issues that were solved by blowing away some old node_modules. This variable exists and is used all over.

OK, I was just copy-pasta-ing that from Circle-CI, so we should see if we can blow away that cache.

pdehaan · 2016-10-18T16:45:45Z

frontend/src/app/components/EmailFooter.js

+  }
+
+  submitSucceeded() {
+    return this.props.forms.email && this.props.forms.email.submitSucceeded;


I think we have some failing Circle-CI tests due to this line:

1) app/components/View should pass its props to child components: TypeError: Cannot read property 'email' of undefined at EmailFooter.submitSucceeded (src/app/components/EmailFooter.js:17:12) at EmailFooter.getClassNames (src/app/components/EmailFooter.js:63:21) at EmailFooter.render (src/app/components/EmailFooter.js:69:28) at /home/ubuntu/testpilot/node_modules/react/lib/ReactCompositeComponent.js:793:21 at measureLifeCyclePerf (/home/ubuntu/testpilot/node_modules/react/lib/ReactCompositeComponent.js:74:12)

Yeah, I haven't touched tests yet. As i noted in comment 0, I put this up there so John could do a UX review of it while I worked.

pdehaan · 2016-10-18T16:56:19Z

@johngruen Is that the correct email icon image? I vaguely recall a conversation at mozilla-services/screenshots#1650 (comment) where we were looking for a simpler image, due to poor "glance-ability".
Not sure if that applies here as well, or if it was just more of a PageShot icon consistency thing.

johngruen · 2016-10-18T17:56:18Z

@pdehaan i think this one is good here now, it's internally consistent with our other email message

lmorchard · 2016-10-19T20:39:38Z

frontend/src/app/components/EmailForm.js

+    );
+  }
+
+  subscribeToBasket(values) {


FWIW, we have a subscribeToBasket helper in containers/App.js that we pass in props for components/EmailDialog.js and mock out in the test. Could probably use the same thing here?

Yup, most of the code is lifted from there. As I mentioned in comment 0, I'm trying to appropriately encapsulate all the functionality in the form so it can be reused elsewhere without dependency. I'll likely remove that code when addressing #1551.

chuckharmston · 2016-10-19T21:58:28Z

So, there's a really weird thing here:

redux-form requires that your form components be connected through a decorator that wires it to the state.
Enzyme's shallow rendering does not work when there are nested connected components; it throws an exception complaining that the store isn't present in the context (which of course it isn't; we're trying to make components that can be tested in isolation).
Passing context manually doesn't work, for reasons I don't have my head around yet.

This is a problem, because this component (and other forms we might create with the library) will be a child of all of our views. Some related bugs:

enzymejs/enzyme#183
enzymejs/chai-enzyme#13
enzymejs/enzyme#435
enzymejs/enzyme#472

I spent most of the day getting to this point wherein the existing tests pass with my patch. I had to do some hackery, effectively not rendering the email footer in all tests. This is less than ideal, but @clouserw mentioned that there's some urgency in this bug, so I'm putting this out there for review now without any additional tests.

Some ideas:

I'm going to keep trying to figure out why passing the context manually isn't working for me. The key is likely somewhere in there.
I might be able to skip the reduxForm entirely if we're able to pass the store down manually. But it does a lot, and this is likely not feasible.
You might be able to wrap the component being tested in <Provider>, but many of Enzyme's APIs are restricted to the root-mounted component, so we'd have to drastically rewrite existing tests.
We can skip on redux-form entirely. It's popular, comprehensive, and well-supported so I'd hate to do that, but it is an option.
We can hide these connected components in tests. This is also awful.

@lmorchard @dannycoates: would love to hear your thoughts here.

chuckharmston · 2016-10-19T22:10:10Z

I'm going to keep trying to figure out why passing the context manually isn't working for me. The key is likely somewhere in there.

Promising: I was able to get this working by setting the static contextTypes on ExperimentDetail.

lmorchard · 2016-10-19T22:10:59Z

We can skip on redux-form entirely. It's popular, comprehensive, and well-supported so I'd hate to do that, but it is an option.

I'd lean toward this. It's not a very complex form, is it? Maybe pass on this library until we need it?

chuckharmston · 2016-10-19T22:20:13Z

I'd lean toward this. It's not a very complex form, is it? Maybe pass on this library until we need it?

redux-form makes life a lot easier: it manages fields (which is very complex when you link them to a store), state, and validation. This is drastically less (and more grokkable) code than it would have otherwise; definitely a good library to use as far as hygiene goes.

If the problem it creates is related to our testing stack, then I'd say that some investigating is warranted, at the least.

chuckharmston · 2016-10-19T23:19:48Z

I'm going to rewrite it without the library. For posterity, the commit with the passing tests was 413ddfb.

chuckharmston · 2016-10-20T00:52:08Z

Alright, this is updated, rolling it by hand.

I'm not at all thrilled with the approach, especially compared to the relative simplicity of redux-form, but it does work. I'll get back to it tomorrow.

chuckharmston · 2016-10-20T18:11:34Z

frontend/src/app/actions/newsletter-form.js

+  }
+})
+
+export default Object.assign({}, actions, subscribeActions);


This is sort of hacky, but it works.

The pattern is a little clearer when using vanilla redux for actions: http://redux.js.org/docs/advanced/AsyncActions.html#async-action-creators

chuckharmston · 2016-10-20T18:12:47Z

frontend/src/app/containers/App.js

-      if (!installed) { pollAddon(); }
-    }
-  }),
-  (stateProps, dispatchProps, ownProps) => Object.assign({


This was getting unwieldy. I broke into parts to make it clearer and to make the actual connect call a one-liner.

chuckharmston · 2016-10-20T18:25:26Z

The rewrite is now finished, other than the tests.

@clouserw expressed interest in merging it without tests before the freeze, so I'd like to get this reviewed now. In the interim I'll get started on them.

r? @lmorchard @dannycoates

chuckharmston · 2016-10-20T18:26:33Z

frontend/src/app/actions/newsletter-form.js

+);
+
+const subscribeActions = createActions({
+  newsletterFormSubscribe: (dispatch, email) => {


dispatch is added in mapDispatchToProps here: https://github.com/mozilla/testpilot/pull/1620/files#diff-d1e3fb4bae03a0b6b298c56e7110d06fR159

I do not like this but I'm not going to block it

Yeah, I don't really like actions dispatching other actions like this.

Maybe the form should dispatch the newsletterFormSetSubmitting and newsletterFormSubscribe actions one after the other itself?

And then you can just yield an appropriate action payload describing success/fail at the end of the fetch without dispatching further actions. We kind of do this in fetchExperiments, but sadly without good error handling.

But, this payload could just be for one single action (newsletterFormSubscribe) that the reducer handles appropriately based on whether it has a statusSuccess==true or statusFailed==true property - or something like this.

@lmorchard so, I think it does make sense for actions to dispatch other actions; this pattern is even featured in Redux's docs: http://redux.js.org/docs/advanced/AsyncActions.html#async-action-creators

The problem is in the redux-actions library, which cuts some boilerplate at the cost of functionality.

Eh, yeah, I think it just rubs me the wrong way for reasons I'm unable to really explain. But, this looks like a pretty textbook example from the docs. So, may just be I need to soak my head in more docs

dannycoates

LGTM. Needs a followup for tests.

dannycoates · 2016-10-20T19:48:18Z

frontend/src/app/actions/newsletter-form.js

+);
+
+const subscribeActions = createActions({
+  newsletterFormSubscribe: (dispatch, email) => {


I do not like this but I'm not going to block it

dannycoates · 2016-10-20T20:00:41Z

frontend/src/app/components/NewsletterForm.js

+    super(props);
+    this.handleEmailChange = this.handleEmailChange.bind(this);
+    this.handlePrivacyClick = this.handlePrivacyClick.bind(this);
+    this.handleSubmit = this.handleSubmit.bind(this);


we aren't consistent in this repo wrt this vs e => this.foo(e) and should eventually decide on one or the other

dannycoates · 2016-10-20T20:06:27Z

frontend/src/app/actions/newsletter-form.js

+      .then(() => dispatch(actions.newsletterFormSetSucceeded()))
+      .catch(() => dispatch(actions.newsletterFormSetFailed()));
+  }
+})


dannycoates · 2016-10-20T20:08:58Z

frontend/src/app/components/NewsletterForm.js

+      </form>
+    );
+  }
+}


Please followup with PropTypes

@johngruen

@johngruen's review doesn't count 😝

chuckharmston added the status: in progress label Oct 15, 2016

johngruen previously requested changes Oct 17, 2016

View reviewed changes

pdehaan reviewed Oct 18, 2016

View reviewed changes

lmorchard reviewed Oct 19, 2016

View reviewed changes

chuckharmston mentioned this pull request Oct 19, 2016

Create and use factory function for store #1634

Closed

Creates and uses factory function for store (closes #1634).

783e6c2

chuckharmston commented Oct 20, 2016

View reviewed changes

dannycoates approved these changes Oct 20, 2016

View reviewed changes

Adds newsletter subscription form to footer (closes #1521).

df1e159

chuckharmston merged commit 035d86f into mozilla:master Oct 20, 2016

chuckharmston removed the status: in progress label Oct 20, 2016

dannycoates mentioned this pull request Oct 20, 2016

pass browserHistory into createStore #1642

Merged

chuckharmston mentioned this pull request Oct 24, 2016

Write tests for newsletter form in the footer. #1640

Closed

Adds newsletter subscription form to footer (closes #1521). #1620

Adds newsletter subscription form to footer (closes #1521). #1620

Conversation

chuckharmston commented Oct 15, 2016 • edited Loading

TODO

Testing

Submitting

Error

Success

Screenshots

Default

After entering a character

When trying to submit with an empty email

When trying to submit with an invalid email

When trying to submit without agreeing to the privacy notice

At medium width

At small width

On success

On error

chuckharmston commented Oct 17, 2016

johngruen left a comment

Choose a reason for hiding this comment

johngruen commented Oct 17, 2016

johngruen commented Oct 17, 2016

johngruen commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdehaan commented Oct 18, 2016

johngruen commented Oct 18, 2016 • edited Loading

lmorchard Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckharmston commented Oct 19, 2016

chuckharmston commented Oct 19, 2016

lmorchard commented Oct 19, 2016

chuckharmston commented Oct 19, 2016

chuckharmston commented Oct 19, 2016

chuckharmston commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckharmston commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard Oct 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannycoates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckharmston commented Oct 15, 2016 •

edited

Loading

johngruen commented Oct 18, 2016 •

edited

Loading

lmorchard Oct 19, 2016 •

edited

Loading

lmorchard Oct 20, 2016 •

edited

Loading