Fix for not including cf-expandables at generation

cfpb / generator-cf

Yeoman generator for Capital Framework

http://cfpb.github.io/capital-framework/getting-started/

Creative Commons Zero v1.0 Universal

9 stars 13 forks source link

Fix for not including cf-expandables at generation #109

Closed Scotchester closed 8 years ago

Scotchester commented 8 years ago

Generated main.js no longer breaks on first run if the cf-expandables component was deselected during generation.

Fixes #108

Notes:

The extra template markup to get this to work is ugly as sin. Happy to take formatting suggestions, either via comments or direct additional commits to this branch.

Review:

@contolini
@ascott1
@jimmynotjim

Scotchester commented 8 years ago

I don't understand the Travis failures at all. I also get totally different test failures locally. Would like to pair on this with someone tomorrow.

Scotchester commented 8 years ago

Also worth noting that Travis is still using these old versions:

$ node --version
v0.10.41
$ npm --version
1.4.29
$ nvm --version
0.23.3

contolini commented 8 years ago

Not sure why the tests are failing (it's happening locally as well). Regardless, shouldn't we leave jQuery and the component counting snippet even if cf-expandables isn't selected? Something like: https://gist.github.com/contolini/4c81fa0db1f8da52bf33

Scotchester commented 8 years ago

@contolini I actually started with that, and realized that we weren't getting jQuery as a dependency without cf-expandables. We'd have to add it as a standalone dependency to the generated project, which I don't think we want to do, do we?

contolini commented 8 years ago

Great catch. After some thought, I vote for something like this: https://gist.github.com/contolini/4c81fa0db1f8da52bf33

Include jQuery as a standalone dependency.
Add a comment explaining how to add JS modules to the project (for folks new to webpack/browserify).
If they didn't select cf-expandables when running the generator, leave the require( 'cf-expandables' ) line but comment it out so it can serve as an example.

contolini commented 8 years ago

The rationale being: If someone doesn't want JS in their final product, it's trivial to delete the <script> tag in index.html.

But if they do want JS, and all we provide them is an empty main.js file, figuring out the global.$ = require( 'jquery' ) line might be a PITA for them.

Scotchester commented 8 years ago

What about wanting JS but not wanting jQuery, though?

contolini commented 8 years ago

Maybe include a comment like this? https://gist.github.com/contolini/4c81fa0db1f8da52bf33#file-cf-js-L3

Scotchester commented 8 years ago

I still think I'd rather not install it by default. I'm not seeing the compelling reason to do that.

contolini commented 8 years ago

Some elucidation from the #fewd room:

i’d like to include it by default to help folks less-experienced with the JS bundling process the people who will want jQuery are less-experienced JS devs and i’m betting they’ll struggle with installing jquery and hooking it into webpack/browserify l33t folks who don’t rely on jquery will open main.js, see the require(‘jquery’) line, understand what it is, and delete it

Scotchester commented 8 years ago

Ok, yeah, I'm convinced. Updating now.

Scotchester commented 8 years ago

Updated and ready for review again!

Scotchester commented 8 years ago

And Travis is unborked! :tada:

contolini commented 8 years ago

:balloon: