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

Project complete #12

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

Project complete #12

wants to merge 12 commits into from

Conversation

DanGRT
Copy link

@DanGRT DanGRT commented Oct 15, 2018

No description provided.


import '../styles/App.scss';

class App extends React.Component {
constructor(){
super();

this.getMenu = this.getMenu.bind(this)
Copy link
Contributor

Choose a reason for hiding this comment

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

getMenu gets called as a method and does not need binding as a result


const currentOrder = {
total: this.state.currentOrder.total + foodItem[menuItem.id].totalPrice,
items: Object.assign(this.state.currentOrder.items, foodItem)
Copy link
Contributor

Choose a reason for hiding this comment

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

IT would be better to us Object.assign({}, this.state.currentOrder.items, foodItem) as the current implementation mutates state

@dmitrigrabov
Copy link
Contributor

Great work. I would suggest keeping the order data structure simpler and just store ids of items ordered and their quantities. Prices and descriptions can then be obtained from the menu object when needed. It prevents data duplication and keeps things simpler. By duplicating data you run the risk of introducing complexity and multiple sources of truth. This could be become significant when working with larger data structures

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