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

Ada Trader - Marisa Morris #48

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Ada Trader - Marisa Morris #48

wants to merge 8 commits into from

Conversation

marisamorris
Copy link

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? Backbone views help structure the code in that they hold all the logic similar to rails controllers.
Did you use jQuery directly in your Views? How, and why? Yes but i'm not sure i was supposed without the $el.
What was an example of an event you triggered? Why did you need to trigger it? showTrade was an event that I triggered.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? Its different in that Jasmine tests run without support from the browser.

@CheezItMan
Copy link

CheezItMan commented Dec 21, 2017

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Not very many commits, good commit mesages
Comprehension questions Check, You actually didn't use jQuery directly, instead you used this.$. Also in our tests both Jasmine and Minitest don't need the support of the browser. However if you wanted to test the views etc, you would need some way to mock the DOM.
Organization
Models and collections are defined in separate files Check
Code that relies on the DOM is located in or called by $(document).ready Check
Functionality
Quote prices change when clicking Buy and Sell Check
The Trade History updates when buying and selling a quote Check
A user can create an open order using the Order Entry Form Check
An open order removes itself from the open orders and updates the Trade History when fulfilled MISSING you need the Order instances listening for changes in the Quote, see my in-code notes. You had most of what you needed, only lacking a listenTo and event handlers to respond to the event.
General
Has separate views for different parts of the app Check
Uses events (listening/handling and triggering) to manage different behavior in views You have event listing to respond to changes in Quotes, and respond to events in Order, Quote and the forms.
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) You avoided using jQuery directly. I would use the syntax like initialize(params) { as opposed to initialize: function(params), but that's a minor issue. Another minor thing is to prefer use of single quotes over double quotes.
Error handling for the Order Entry Form Well done using validate in the form!
Testing
Has unit tests for models MISING
Overall It looks like you got part of the way through the project but ran into trouble getting the different components to talk to each other. Hopefully my notes give you some ideas what you could have done. You did well setting up views and rendering content, but had trouble extending further than our livecode.

@@ -0,0 +1,33 @@
import Backbone from 'backbone';

const Order = Backbone.Model.extend({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're passing a Quote instance to the Order, you could listen for price changes,

Something like

initialize(params) {
  this.listenTo(this.get('quote'), 'change', (quote) => {
    // code to buy a quote, maybe a trigger to alert the view or quote.
  }
},

buy() {
// Implement this function to increase the price by $1.00
this.set('price', this.get('price') + 1.00);
this.set('buy', true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this accomplishes, quotes just represent the current price of a quote, why would there be a buy attribute?

'click button.btn-sell': 'addSellOrder',
},

addBuyOrder: function(event) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you merge addBuyOrder and sellBuyOrder?

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