Skip to content

Confirmation box#1

Open
n8pickle wants to merge 5 commits into
masterfrom
confirmationBox
Open

Confirmation box#1
n8pickle wants to merge 5 commits into
masterfrom
confirmationBox

Conversation

@n8pickle
Copy link
Copy Markdown
Owner

please check out Mainly the confirmation box. There were a few bug fixes but not many.

@n8pickle n8pickle requested a review from cmpickle October 28, 2019 21:40
// Use IntelliSense to learn about possible attributes.
// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

removing this colon will probably break things... also don't know why you deleted the comment above

Comment thread client/src/App.js
import { InventoryTablesContainer } from "./Area/InventoryTableContainer";

const App = () => {
const AppComp = ({ classes }) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

use full names like AppComponent. AppComp is lazy and it makes the code less readable.


export const ConfirmationBox = ({ open, setOpen, onDeleteHandler }) => {
return (
<div>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should be a Fragment not a div.

const onSubmit = async (sku, amount) => {
await axios
.post("http://localhost:5000/api/inventory/" + sku + "/set/" + amount)
.then(function(response) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This then isn't doing anything. It is also console logging the response. That isn't something you want to do outside of debugging.

console.log(response);
})
.catch(function(error) {
console.log(error);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

1st. This should be console.err()
2nd. It would be better if this were logged to a true logger (more work than necessary here though)
3rd. it's better to not log the errors to the console for the user to read.

})
.catch(function(error) {
setError(true);
console.log(error);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above.

await axios
.post("http://localhost:5000/api/inventory/" + sku + "/add/" + 1)
.then(function(response) {
console.log(response);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above.

console.log(response);
})
.catch(function(error) {
console.log(error);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

})
.catch(function(error) {
setError(true);
console.log(error);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

.get("http://localhost:5000/api/products")
.then(function(response) {
setTables(response.data);
console.log(response);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

fontSize: "10em"
},
body: {
// fontSize: 14
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

delete this

setError
}) => {
const [navToEditProduct, setNavToEditProduct] = React.useState(undefined);
const [incrementProduct, setIncrementProduct] = React.useState(undefined);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't see where this is being used... It looks like it should be deleted

.get("http://localhost:5000/api/products")
.then(function(response) {
setTables(response.data);
console.log(response);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

})
.catch(function(error) {
setError(true);
console.log(error);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

<IconButton
color="inherit"
aria-label="open drawer"
onClick={handleDrawerOpen}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

instead of creating wrapper methods why not call setOpen directly here?

}}
>
<div className={classes.drawerHeader}>
<IconButton onClick={handleDrawerClose}>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

instead of creating wrapper methods why not call setOpen directly here?

<h3 className={classes.searchHead}>Search:</h3>
<input className={classes.searchText}></input>
<Button variant="contained" color="primary" className={classes.searchBtn}>
<SearchIcon></SearchIcon>
Copy link
Copy Markdown
Collaborator

@cmpickle cmpickle Oct 28, 2019

Choose a reason for hiding this comment

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

this could be <SearchIcon />

Comment thread client/src/EditProduct.js
Quantity: location.state.navToEditProduct.quantity,
NotificationQuantity: location.state.navToEditProduct.notificationQuantity,
Price: 0,
Dimensions: "2x1",
Copy link
Copy Markdown
Collaborator

@cmpickle cmpickle Oct 28, 2019

Choose a reason for hiding this comment

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

This probably shouldn't be hard coded... if it isn't used then it really shouldn't be sent over the network anyway. This applies to all these hard coded values here

Comment thread client/src/EditProduct.js
setItemDeleted(true);
};

if (itemDeleted) return <Redirect to="/"></Redirect>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are all three of these state values just to redirect to the root page? If so consolidate them to one state value for navigating to root

Comment thread client/src/EditProduct.js
console.log(response);
})
.catch(function(error) {
// setError(true);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

delete this if not being used

Comment thread client/src/EditProduct.js
location.state.navToEditProduct.sku
)
.then(function(response) {
console.log(response);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

Comment thread client/src/EditProduct.js
})
.catch(function(error) {
// setError(true);
console.log(error);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

Comment thread client/src/NewProduct.js

const NewProductPageComp = ({ classes }) => {
const [newProductForm, setNewProductForm] = React.useState({
ProductName: "product",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Were you intending to put default values?

Comment thread client/src/NewProduct.js
console.log(response);
})
.catch(function(error) {
// setError(true);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

delete if not using

Comment thread client/src/NewProduct.js
await axios
.post("http://localhost:5000/api/products/", newProductForm)
.then(function(response) {
console.log(response);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

Comment thread client/src/NewProduct.js
})
.catch(function(error) {
// setError(true);
console.log(error);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above

Comment thread client/src/NewProduct.js
SetFormSubmitted(true);
};

if (formSubmitted) return <Redirect to="/"></Redirect>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

once again are both these states just to redirect to root? if so consolidate

// Use IntelliSense to find out which attributes exist for C# debugging
// Use hover for the description of the existing attributes
// For further information visit https://github.com/OmniSharp/omnisharp-vscode/blob/master/debugger-launchjson.md
"version": "0.2.0",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

removing this colon will probably break things.. also why are you deleting these comments?

@@ -1 +1 @@
df8c9abfc30add6ff1506303db041d5dde3264b5
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All these files under server/bin and server/obj should be part of your .gitignore not checked in to VC

Copy link
Copy Markdown
Collaborator

@cmpickle cmpickle left a comment

Choose a reason for hiding this comment

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

look at PR comments

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