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

Delivereat app #5

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

Delivereat app #5

wants to merge 38 commits into from

Conversation

r41ph
Copy link

@r41ph r41ph commented Jul 2, 2018

No description provided.

function getOrders() {
let orders = {};

return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice

},

showDish() {
// return menu;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused code can be removed



return {
showMenu() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor naming point. It would be slightly clearer to name this method getMenu as it returns the menu. showMenu potentially implies some display logic

addOrders(order);
res.json(showOrders())
} else {
res.status(404).json({ error: "Order error" });
Copy link
Contributor

Choose a reason for hiding this comment

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

404 generally refers to an missing resource on the server. Here you are checking that an order was sent up. If that is not the case something like 400, meaning Bad Request would be better

res.render('index');
});

// Server stuff
Copy link
Contributor

Choose a reason for hiding this comment

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

Code is no longer in use and can be removed

});
}
ordersHandler(dishId, quantity, price, action) {
// Find if dish is already in orders and get position
Copy link
Contributor

Choose a reason for hiding this comment

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

Liking the comments

const { date, totalAmount, products, menu, order, receiverDeletedOrder } = props;

const reorderHandler = () => {
const date = new Date();
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 great to move date formatting function to an external module. Let me know if you need help with this

<div className="menu-wrapper">
<Link to="/menu"><h5 className="menu__back-to-menu">Back to Menu</h5></Link>
<h1 className="menu__heading">Order History</h1>
<div className={"old-orders__empty " + (Object.keys(props.oldOrders).length > 0 ? "hidden" : "")}>
Copy link
Contributor

Choose a reason for hiding this comment

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

The condition could be written a little shorter using Object.keys(props.oldOrders).length since it will evaluate to true when length is greater than 0

super(props);
this.state = {
menu: {},
deliveryPrice: 2.50
Copy link
Contributor

Choose a reason for hiding this comment

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

Since delivery price does not change is can go into a variable rather than be in state. For example put it on line 3 as const DELIVERY_PRICE = 2.50


function checkoutHandler(order) {
const date = new Date();
const minutes = date.getMinutes() < 10 ? '0' + date.getMinutes() : '' + date.getMinutes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving date formating to own module would allow you to re-use the code here and avoid duplication

function BasketEmpty(props) {
return (
<div id="basket__empty"
className={"basket__empty " + (Object.keys(props.orders).length === 0 ? '' : 'disabled')}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Condition could be written as slightly shorter as !Object.keys(props.orders).length


function BasketOrderSent(props) {
return (
<div className={"basket-order__sent " + (props.orderSent === true ? '' : 'hidden')}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ternary could be slightly shorter using (props.orderSent ? '' : 'hidden'), since it will already check if the value is true

function BasketPriceDisplay(props) {
return (
<div className={"basket__checkout-button_wrapper " + (Object.keys(props.orders).length > 0 ? '' : 'hidden')}>
<div>Order: <strong>{props.orderAmount.toLocaleString('en-gb', { style: 'currency', currency: 'GBP' })}</strong></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

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