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.

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

Merged
merged 18 commits into from Jul 6, 2015
Merged

Conversation

frenchbread
Copy link
Contributor

Here is settings.json file example (elasticsearch.host parameter is required for app to fetch analytics data):

{
  "api_umbrella": {
    "api_key": "...",
    "auth_token": "...",
    "base_url": "https://.../api-umbrella/"
  },
  "elasticsearch": {
    "host": "http://...:14002"
  }
}

@@ -0,0 +1,70 @@
es = function () {
Copy link
Contributor

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']);
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

bajiat added a commit that referenced this pull request Jul 6, 2015
@bajiat bajiat merged commit f2d21d2 into develop Jul 6, 2015
@bajiat bajiat removed the in progress label Jul 6, 2015
@bajiat bajiat deleted the feature/chart-code-refactoring branch July 6, 2015 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants