New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make template customizable. #7

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@nkovacs
Contributor

nkovacs commented Nov 24, 2015

Fixes #6

I only tested compile with the new option. I also tested two examples in the globalize repo (app-npm-webpack and globalize-compiler) without the new option.

Btw, npm run test didn't work properly for me. jshint succeeded, even though there were errors, and when I ran jshint manually, it did report them.

Show outdated Hide outdated lib/compile.js
.map(function( dependency ) {
return "var " + dependency + " = Globalize._" + dependency + ";";
}).join( "\n" );
properties.compiled = dependenciesVars + "\n" + properties.compiled;

This comment has been minimized.

@rxaviers

rxaviers Nov 24, 2015

Member

Any better name than compiled for this one? code?

Also, could you remove properties? It was previously used as a placeholder for the .replace() substitutions below, but not anymore.

@rxaviers

rxaviers Nov 24, 2015

Member

Any better name than compiled for this one? code?

Also, could you remove properties? It was previously used as a placeholder for the .replace() substitutions below, but not anymore.

This comment has been minimized.

@nkovacs

nkovacs Nov 24, 2015

Contributor

It's still used for the template function, but I could change its signature to function (code, dependencies).

@nkovacs

nkovacs Nov 24, 2015

Contributor

It's still used for the template function, but I could change its signature to function (code, dependencies).

This comment has been minimized.

@rxaviers

rxaviers Nov 24, 2015

Member

function (code, dependencies) sounds good to me or preferably function ({code: code, dependencies: dependencies}).

@rxaviers

rxaviers Nov 24, 2015

Member

function (code, dependencies) sounds good to me or preferably function ({code: code, dependencies: dependencies}).

Show outdated Hide outdated lib/compile.js
typeof args[args.length - 1].template === "function" ) {
templateFn = args[args.length - 1].template;
}

This comment has been minimized.

@rxaviers

rxaviers Nov 24, 2015

Member

I think we should simplify the API. Let's abort the compiler( formatterOrParser, ... ) and stick with what you have documented.

@rxaviers

rxaviers Nov 24, 2015

Member

I think we should simplify the API. Let's abort the compiler( formatterOrParser, ... ) and stick with what you have documented.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 24, 2015

Member

It's looking great so far. I've left a couple of comments above.

Member

rxaviers commented Nov 24, 2015

It's looking great so far. I've left a couple of comments above.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 26, 2015

Member

Bravo. It's looking great now. One last thing, you need to sign jQuery Foundation CLA http://contribute.jquery.org/CLA/.

PS: Ideally we should include one more test asserting the new template option, e.g., passing a template and asserting that has been used to generate the compiled bundle.

Member

rxaviers commented Nov 26, 2015

Bravo. It's looking great now. One last thing, you need to sign jQuery Foundation CLA http://contribute.jquery.org/CLA/.

PS: Ideally we should include one more test asserting the new template option, e.g., passing a template and asserting that has been used to generate the compiled bundle.

@nkovacs

This comment has been minimized.

Show comment
Hide comment
@nkovacs

nkovacs Nov 26, 2015

Contributor

I signed the CLA.

I don't have time to write the test now.

Contributor

nkovacs commented Nov 26, 2015

I signed the CLA.

I don't have time to write the test now.

@rxaviers rxaviers closed this in be0bce8 Nov 26, 2015

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 26, 2015

Member

Thanks!

Member

rxaviers commented Nov 26, 2015

Thanks!

rxaviers added a commit that referenced this pull request Nov 26, 2015

0.2.0-pre
The fix for #6 requires minor update.

Ref #6
Ref #7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment