-
Notifications
You must be signed in to change notification settings - Fork 60
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
Milestone pull request submission #45
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,64 @@ | |||
<br> |
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.
Try to reduce the usage of <br>
in your views
Suggestions
Error
|
const routes = require('./routes/routes') | ||
const dbConfig = require('./config/dbConfig') | ||
|
||
var AlphaVantageAPI = require('alpha-vantage-cli').AlphaVantageAPI |
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.
Please explain how do you use this API on your readme
models/position.js
Outdated
price: Number, | ||
buyDate: String, | ||
|
||
user: [{ |
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.
Why one position can reference to multiple user
?
models/position.js
Outdated
//Get current price of Stock | ||
var AlphaVantageAPI = require('alpha-vantage-cli').AlphaVantageAPI; | ||
|
||
var yourApiKey = 'WMIBV3Q29V0HHRV9'; |
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.
Look at how your classmates are hiding their API key
models/position.js
Outdated
var alphaVantageAPI = new AlphaVantageAPI(yourApiKey, 'compact', true); | ||
var intradayData | ||
|
||
// alphaVantageAPI.getIntradayData(this.ticker, '1min') |
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.
Please remove all the commented codes once you don't use it anymore
@@ -0,0 +1,17 @@ | |||
// const mongoose = require('mongoose') |
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.
What is this model for? Because I don't see it in your ERD
views/layouts/main.handlebars
Outdated
|
||
|
||
<script src="https://code.jquery.com/jquery-3.2.1.min.js" charset="utf-8"></script> | ||
<script src="https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.0/Chart.bundle.min.js" charset="utf-8"></script> |
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.
What is this plugin for?
views/positions.handlebars
Outdated
</thead> | ||
|
||
{{#each position}} | ||
<tbody class='positiontable'> |
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.
<tbody>
should be outside of the {{#each }}
loop
controllers/homeController.js
Outdated
|
||
newPosition.save(function(err){ | ||
if(err) throw err | ||
Position.find({},function(err,result){ |
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.
Why do you need to find()
all the positions and redirect to another page?
Deliverable Submission
Please describe your comfort and completeness levels before submitting.
Comfort Level (1-5): 3
Completeness Level (1-5):2.5
What did you think of this deliverable?: