-
Notifications
You must be signed in to change notification settings - Fork 43
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
Kimberley Zell - PIPES - Ada Trader #30
base: master
Are you sure you want to change the base?
Conversation
…/sell rather than the current price when the action occured.
Ada TraderWhat We're Looking For
|
const orderFormView = new OrderFormView({ | ||
el: '#order-entry-form', | ||
orderList: orders, | ||
quoteList: quotes |
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.
I like that the OrderFormView
knows about the orderList
and the quoteList
. This makes sense - these are the things it needs in order to do its job. I saw many students attempt to use the bus to get this information around, which ends up being a little clunky.
|
||
executeOrder() { | ||
console.log('execute order is being called'); | ||
let quote = this.model.get('quote'); |
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.
I think this might make more sense as a function defined on the Order
model, since it only does data manipulation, and doesn't touch the DOM at all. To make sure the view is properly removed, you could add the following line to initialize()
above:
this.listenTo(this.model, 'destroy', this.remove);
this.model.destroy(); | ||
this.remove(); | ||
} | ||
} |
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.
The last thing that needs to happen here is to add a trade to the trade history. Based on your code elsewhere, it looks like triggering a trade
event on the bus would set things in motion properly.
buy(event) { | ||
console.log(event); | ||
this.bus.trigger('trade', { | ||
//this is what trade is |
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.
The code here in buy()
is almost identical to what you've written in sell()
below. Could these be consolidated somehow?
}, | ||
buy(event) { | ||
console.log(event); | ||
this.bus.trigger('trade', { |
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.
This is a good use of the bus. Many different sources (the different quotes) can produce the same event, and you want to be able to listen for them in one central place.
Ada Trader
Congratulations! You're submitting your assignment!
Comprehension Questions