Skip to content

Milestone pull request submission#45

Open
empludo wants to merge 21 commits into
wdi-sg:masterfrom
empludo:master
Open

Milestone pull request submission#45
empludo wants to merge 21 commits into
wdi-sg:masterfrom
empludo:master

Conversation

@empludo

@empludo empludo commented Jan 3, 2018

Copy link
Copy Markdown

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?:

@@ -0,0 +1,64 @@
<br>

Copy link
Copy Markdown
Contributor

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

@primaulia

primaulia commented Jan 11, 2018

Copy link
Copy Markdown
Contributor

Suggestions

  • Please make sure to fix the authentication flow of your project
  • Add a simple validation for all of your form submission
  • .DS_Store should be ignored from your project

Error

  • Sell button doesn't work on heroku

Comment thread app.js
const routes = require('./routes/routes')
const dbConfig = require('./config/dbConfig')

var AlphaVantageAPI = require('alpha-vantage-cli').AlphaVantageAPI

Copy link
Copy Markdown
Contributor

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

Comment thread models/position.js Outdated
price: Number,
buyDate: String,

user: [{

Copy link
Copy Markdown
Contributor

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?

Comment thread models/position.js Outdated
//Get current price of Stock
var AlphaVantageAPI = require('alpha-vantage-cli').AlphaVantageAPI;

var yourApiKey = 'WMIBV3Q29V0HHRV9';

Copy link
Copy Markdown
Contributor

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

Comment thread models/position.js
var alphaVantageAPI = new AlphaVantageAPI(yourApiKey, 'compact', true);
var intradayData

// alphaVantageAPI.getIntradayData(this.ticker, '1min')

Copy link
Copy Markdown
Contributor

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

Comment thread models/summary.js
@@ -0,0 +1,17 @@
// const mongoose = require('mongoose')

Copy link
Copy Markdown
Contributor

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

Comment thread 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>

Copy link
Copy Markdown
Contributor

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?

Comment thread views/positions.handlebars Outdated
  </thead>

{{#each position}}
<tbody class='positiontable'>

Copy link
Copy Markdown
Contributor

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

Comment thread controllers/homeController.js Outdated

newPosition.save(function(err){
if(err) throw err
Position.find({},function(err,result){

Copy link
Copy Markdown
Contributor

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?

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.

2 participants