Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Region Helper - Paper - MERC-2432 #118

Merged
merged 2 commits into from
Jun 22, 2017
Merged

Region Helper - Paper - MERC-2432 #118

merged 2 commits into from
Jun 22, 2017

Conversation

tabayomi
Copy link
Contributor

@tabayomi tabayomi commented Jun 5, 2017 •
edited
Loading

What:
Add an attribute to Paper to use for rendering store content.

Also - converts Paper to using ES6 style classes.

@tabayomi tabayomi force-pushed the instance-of-paper-MERC-2432 branch from 284cfe7 to edef0a5 Compare June 5, 2017 19:57
mcampa
mcampa reviewed Jun 5, 2017
View reviewed changes
index.js Outdated
}
_.each(helpers, function (helper) {
helper(self);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this code to

helpers.forEach(helper => helper(this));

index.js Outdated
var self = this;
class Paper {
constructor(settings, themeSettings, assembler) {
let self = this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need self anymore. You can use this directly.

index.js Outdated
eval('template = ' + precompiled);
self.handlebars.templates[path] = self.handlebars.template(template);
Async.parallel([
function (next) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make these arrow functions?

index.js Outdated
if (path[0] !== '/') {
path = '/' + path;
}
if (path.substr(0, 8) === '/assets/') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use RegEx path.match(/^\/assets\//)

index.js Outdated
var self = this;
// Make translations available to the helpers
translator = Translator.create(data.acceptLanguage, translations);
let instance = new Paper(data.context.settings, data.context.themeSettings, assembler);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const instead

index.js Outdated
* @param {Function} callback
*/
loadTemplates(path, callback) {
let self = this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. You don't need this anymore. Just make sure to convert all anonymous functions inside this block to use arrow functions

index.js Outdated
let cdnUrl = this.settings['cdn_url'] || '';
let versionId = this.settings['theme_version_id'];
let sessionId = this.settings['theme_session_id'];
let protocolMatch = /(.*!?:)/;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use const on variables that are not reassigned

index.js Outdated

return [cdnUrl, 'stencil', versionId, path].join('/');
};
_.each(this.decorators, function (decorator) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

this.decorators.forEach(decorator => {...});

index.js Outdated
* @return {String|Object}
*/
renderTheme(templatePath, data) {
let html,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one let per variable

index.js Outdated
return output;
};
if (data.remote) {
data.context = _.extend({}, data.context, data.remote_data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use Object.assign

@tabayomi tabayomi force-pushed the instance-of-paper-MERC-2432 branch from edef0a5 to f0d4121 Compare June 5, 2017 22:20
@tabayomi tabayomi mentioned this pull request Jun 5, 2017
Closed
mattolson
mattolson reviewed Jun 5, 2017
View reviewed changes
.eslintrc Outdated
@@ -8,6 +8,7 @@
},
"env": {
"node": true,
"es6": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that we can do this without upgrading Stapler. @mcampa

Copy link
Contributor Author

@tabayomi tabayomi Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can remove that. I added it so eslint would stop fussing about Promises, but since I've removed Promises from the code, it's not necessary anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use constructor in es5? I thought that was an es6 thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually already enabled for es6. The original file has this in there.

{
  "parserOptions": {
    "ecmaVersion": 6,
    "sourceType": "module"
  },

That addition was only about making Promises global.

@tabayomi tabayomi force-pushed the instance-of-paper-MERC-2432 branch from f0d4121 to f1deac9 Compare June 5, 2017 23:37
@mcampa
Copy link
Contributor

mcampa commented Jun 5, 2017

Please add node Can you please add node 4 & 6 to travis.yml file?

@tabayomi tabayomi force-pushed the instance-of-paper-MERC-2432 branch 2 times, most recently from dcfa65b to adfd154 Compare June 16, 2017 21:11
@tabayomi tabayomi changed the title Create constructor to allow full access to helpers inside Paper - MERC-2432 Region Helper - Paper - MERC-2432 Jun 16, 2017
mcampa
mcampa reviewed Jun 19, 2017
View reviewed changes
index.js Outdated
this.settings = settings || {};
this.themeSettings = themeSettings || {};
this.assembler = assembler || {};
this.renderingContext = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you name this differently, to be specific to content blocks? Like maybe contentBlocksContext

@tabayomi tabayomi force-pushed the instance-of-paper-MERC-2432 branch from adfd154 to 56c8e0c Compare June 19, 2017 20:30
@tabayomi tabayomi mentioned this pull request Jun 19, 2017
Merged
2 tasks
mcampa
mcampa approved these changes Jun 19, 2017
View reviewed changes
Copy link
Contributor

@mcampa mcampa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. just make sure everything works with the cli and stapler before merging

@pedelman pedelman merged commit 7313ec7 into bigcommerce:master Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattolson mattolson mattolson left review comments

@mcampa mcampa mcampa approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tabayomi @mcampa @mattolson @pedelman