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

phoebedg delivereat #2

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

Conversation

phoebedg
Copy link

No description provided.

if (item) {
return res.json(menu);
} else {
return res.status(404).json({ error: "item not found" });
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

});

app.post("/api/order", function(req, res) {
const orderId = `order-${storage.id++}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

might be simpler use just the id number rather than append to a string

res.status(201).json(orderhistory);
});

// Use when not on Heroku app
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is no longer needed and can be deleted

matchingOrder.quantity += order.quantity;
matchingOrder.price += order.price;

const ordersWithoutCurrentOrder = this.state.orders.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

}

handleClearHistory(key) {
// const tempOrderHistory = [...this.state.orderhistory];
Copy link
Contributor

Choose a reason for hiding this comment

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

code can be deleted

}

render(){
render() {
console.log("set order hist:", this.state.orderhistory);
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good remove console.logs from commit

View Order History
</button>
<div className="main">
{Object.keys(this.state.orderhistory).length === 0 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be written slightly shorter as !Object.keys(this.state.orderhistory).length since if length is 0, it will be falsy

closeWasClicked={this.closeWasClicked}
/>
)}
{Object.keys(this.state.orders).length === 0 ? null : (
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

componentDidMount() {
const { orders } = this.props;
let prices = Array.from(orders).map(el => el.price);
let totalPrice = prices.reduce(function(acc, item) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

let totalPrice = prices.reduce(function(acc, item) {
return acc + item;
}, 0);
let priceWithDelivery = totalPrice + 2.95;
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 good to extract th delivery charge into variable and use that. That way it will be clearer what 2.95 refers from the variable name. Also, the variable could be replaced in one place if it had to be changed

let totalPrice = prices.reduce(function(acc, item) {
return acc + item;
}, 0);
let priceWithDelivery = totalPrice + 2.95;
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

@@ -0,0 +1,65 @@
import React from "react";

class OrderFormItem extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be a stateless component, since it has no state. Also, commented out code should be removed

@@ -0,0 +1,29 @@
import React from "react";

class OrderHistoryCloseButton extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a stateless component

@@ -0,0 +1,58 @@
exports.storage = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This data object can be imported into the server. This would be quite nice as the data would be separate from code and keep server.js cleaner

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