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

Carets - Maria - ada-trader #38

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

Carets - Maria - ada-trader #38

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 18, 2017

Ada Trader

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
How do Backbone Views help structure your code? The views help with separation of concerns and acted as a facilitator between data, like functions in the models, and between the DOM.
Did you use jQuery directly in your Views? How, and why? I did via the $el which is a jQuery object. I did so so that the views would render.
What was an example of an event you triggered? Why did you need to trigger it? An event I triggered was listing the quotes. That list change was triggered when a user clicked the buttons.
In what was is unit testing in JavaScript similar to unit testing in Ruby? In what ways is it different? The tests are similar in that they had validations and you tested for each function that was built.

@CheezItMan
Copy link

Ada Trader

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Not very many commits, good commit mesages
Comprehension questions Check, however you didn't actually manually trigger any events.
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 NOT WORKING
An open order removes itself from the open orders and updates the Trade History when fulfilled MISSING
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 the QuoteListView listening for changes in the collection and models and re-rendering. However You don't have the OrderListView working or listening to the form, or any component listening for changes in the Quote instances.
Practices good standards for views (ES6 syntax, doesn't directly use jQuery, returns this in render) Check, but you're not utilizing the form for the OrderListView.
Error handling for the Order Entry Form MISSING
Testing
Has unit tests for models MISSING
Overall You've got a lot incomplete here. You set up a view for the Quotes and QuoteListView perfectly and append trades to the list well. You didn't complete the OrderView or OrderListView or complete any testing. You can see some notes I put in your code as to what could be done to get it working. The biggest error is the fact that both QuoteListView and OrderListView are both tied to the same main html element which means they could theoretically interfere with each other. See my comments in your code.

});

const orderListView = new OrderListView({
model: orders,

Choose a reason for hiding this comment

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

I would suggest passing quotes in to the OrderListView and then the view can listen for changes in quotes and check to see if it's time to execute an order.

const orderListView = new OrderListView({
model: orders,
template: _.template($('#order-template').html()),
el: 'main'

Choose a reason for hiding this comment

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

Also note that both views represent the SAME element in the DOM, this means it's possible they could interfere with each other.

Instead I would suggest making the QuoteListView's el element be #quotes-container and the OrderListView's el be #order-workspace.

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