Initial Development

[localjo]

Do we need to be concerned at all about what happens when a visitor has JavaScript disabled, or are we leaving that concern up to the developer who use this?

[localjo]

Concerning email validation, modern browsers validate <input type="email">, and Universe and the other platforms we support should be validating the email addresses server-side as well (not actually sure if they do, but I'd be surprised if they didn't). We could rely on those layers of validation instead of validating the email addresses in JavaScript. @pushred what are your thoughts on that?

[pushred]

Yeah I'm fine with relying on the server's own validation / messaging. It's a request, but no biggie.

[localjo]

I refactored this as a node-flavored module and then used Browserify's standalone option to build it as a UMD module. @pushred if you want to give this a whirl, you can just add "subscribe-email": "git://github.com/blocks/subscribe-email#4bc3e0" to an existing project's package.json. You can then require it using Browserify (or however you want), and initialize it on a form element with;

SubscribeEmail.init({
    element: '#subscribe-form',
    template: true,
    service: 'universe',
    key: 'd54e8487-e44e-4c6f-bdd7-6ab9c2eae1e9'
});

I think this resolves #2

[pushred]

Hmm sadly no.. I'm still seeing all the errors I listed there when Browserify is bundling my project. Confirmed that node_modules/subscribe_email/package.json has "main": "build/subscribe-email.js". Also tried the same entry point with Beefy and it stops with the same issue:

Error: Cannot find module './templates/BEM-with-messaging.hbs' from '/project/node_modules/subscribe-email/build'

[localjo]

Figuring out how to use XDomainRequest in IE9 (required for SendGrid) was a bit tricky. IE9 was aborting requests for no obvious reason. This is apparently a common issue, but the solutions that have worked for others didn't quite work here, but building on those, I finally got it working.

[localjo]

Just added some basic Mocha tests. They need some work, but they're enough to get a glance of whether or not the module is working in various browsers.

Obviously the tests need to be better integrated into the workflow, but this is a workable start.

[localjo]

To run tests you need to;

Export the path to selenium-sever-standalone jar to variable SELENIUM. export SELENIUM='<path to the jar>/selenium-server-standalone-2.33.0.jar'

Run gulp test, which will start the local server and the BrowserStack local tunnel and then run the Mocha tests.

[pushred]

Workflow thing: should keep these built versions out of the branches I think, and only generate them when you're committing a version bump for npm/Bower. Just can add good bit of weight in the Changed view with the dependencies.

[pushred]

Actually the plan was to attach those to the GitHub releases, so can go in there. Guessing they're committed right now because they're the entry point currently.

[pushred]

Aside from the template discussion above there's a couple other consistency things I'd like to flag:

• There's a template element and option related to responses but the event is called message. They're also called messages over in alert. Message seems appropriate, similar in practice to Web Messaging.
• alert has a BEM block container but subscribe doesn't, that's basically a byproduct of the html vs. appendTo method of rendering the templates. I'm a bit torn on which approach is better. On the one hand such "placeholder" containers can be given styling that mitigates the flash of nothingness. They can also include title headings and other things that can be pretty variable depending on the situation. On the other hand they can sometimes complicate styling, particularly when combinator selectors are involved. But maybe :empty and classes can help there.
• Relatively minor, but just noticed you titled the project "Subscription Email Widget" in Browserstack and it's described as a "module" in the README. Let's definitely clear this up when we make the site, confusion around this kind of thing is common. Just wanted to point it out!

These small details can just add up and make for a lot of dissonance when working within a system, so let's just be conscious of that as we build more of these things.

[pushred]

What do you think of making alerter responsible for the response messages?

[localjo]

Added some commits that addressed feedback. Here are some remaining things to do;

• [x] Improve insertion of rendered template.

@pushred Like you mentioned, this is done differently in alerter, which was originally a byproduct of the different approaches to overriding the template. I think ideally, we would have a neutral placeholder element, which could contain anything necessary to avoid a flash of nothingness, and then that placeholder would be completely replaced by the rendered template without any extra wrapping elements (alerter currently has a wrapping div). I ran into some challenges when trying to make this change, related to DOM behavior, but with a little bit of time, those can be worked out. I think these modules can and should insert content into the DOM in the same way now.

• [x] Mock API Responses using Sinon's FakeServer or VCR
• [x] Add Additional Tests for
  • [x] Template overrides (or passing a Handlebars template if you go that route)
  • [x] Passing a jQuery element vs. a DOM selector
  • [x] Event emitter
• [x] Make alerter responsible for displaying messaging
• [x] Improve consistency in how public/private methods are defined
• [x] Clean up README and options related to responses and messaging

After all that stuff is done, and reviewed;

• [x] Publish a release to GitHub
• [x] Update package.json and publish a release to npm
• [x] Add bower.json and publish a release to Bower

[localjo]

Got API responses mocked with Sinon's fakeServer for CORS requests and sinon.stub() for JSONP. All tests are passing except IE9, where I'm running into problems with fakeServer not working. I spent the afternoon debugging, and opened an issue here: https://github.com/cjohansen/Sinon.JS/issues/584

[localjo]

Finally solved the Sinon issue (see issue link above). Almost all tests are passing now. The only automated tests that aren't passing are Mocha tests in IE9 (seems they aren't running at all) @joanniclaborde Did you have any problem with that in your tests? I'm using your code to run the Mocha tests.

[joanniclaborde]

Yeah I had trouble running the tests in IE9 too, and I couldn't find a solution. The tests would run once in a while, but most of the time nothing would happen. I gave up on it for now...

[localjo]

247 tests passing! \o/ Looks like all tests are passing in all browsers.

[pushred]

Trying the latest locally I'm running into that issue where the Handlebars precompiled template source is rendered into the page where the alert's supposed to go. Checking blocks-subscribe/node_modules/blocks-alert/package.json I'm seeing 0.0.2. Is another release needed to fix that issue?

[localjo]

@pushred I found out that that issue is caused by the template being browserified twice. Whether that's because it's in a gulp task and package.json, or applied to the dependent module and then again, globally, from the parent module. So that should help narrow down why you're seeing that, but I'm not able to replicate that finding. Also, which package are you seeing @0.0.2?

[pushred]

Oops I meant 1.0.2, gotta get used to this insane 1.x versioning! I just removed the global transform I had in my parent project, but now Browserify gets stuck:

Running "browserify:scripts" (browserify) task
>> Error: Parsing file /Users/eric/dev/sites/funimpact.org/node_modules/blocks-subscribe/src/subscribe-form.hbs: Line 5: Expected corresponding XJS closing tag for input

Did you forget to push up the transform in this module's config?

[pushred]

Yeah it's all good once I add that.