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
Feature/chart code refactoring #198
Conversation
Conflicts: server/methods/elasticSearch.js
@@ -0,0 +1,70 @@ | |||
es = function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
es
might be improved by adding more semantic details. E.g. ApiUmbrellaElastic.
host: Meteor.settings.elasticsearch.host | ||
}); | ||
|
||
EsClient = Async.wrap(EsClientSource, ['index', 'search']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comments to describe this line.
|
||
EsClient = Async.wrap(EsClientSource, ['index', 'search']); | ||
|
||
// index: index provided within the query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use JsDoc style @params here, as well as the general JsDoc comment style.
var chartDataArr = []; | ||
var monthFrames = { | ||
monthStart: 1, | ||
monthEnd : 31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Month length should not be hard-coded. Here are a couple of possible methods for getting the number of days in any given month:
I believe our project already contains Moment.js, which has an elegant API for JS dates.
Here is settings.json file example (elasticsearch.host parameter is required for app to fetch analytics data):