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

Checkout #8

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

Conversation

navya-shajan
Copy link

checkout and order confirmation.

@correttojs

Copy link
Contributor

@MrTim MrTim left a comment

Choose a reason for hiding this comment

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

Good work! left some comments and suggestions where could be improved.
One thing in general I see in your code - not consistent code style and broken indentation. This aspect of the code is very important and would be really helpful (for you) if you'll try to pay a little bit more attention there :)

export function Checkout() {
return <div>Checkout</div>;
return <div>
<Form />
Copy link
Contributor

Choose a reason for hiding this comment

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

something wrong with indentation. And check it in all the files and code, because there are some places below when it's broken.

display: block;
margin: 5px;
`;

Copy link
Contributor

Choose a reason for hiding this comment

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

not needed empty line

const [customer, setCustomer] = useState();
const history = useHistory();
const handleChange = (e)=>{
const name=e.target.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

pay attention to the code style, this is super important when you're submitting your code for review.
Here you don't put spaces around = and =>.
Suggestion (one more time): install Prettier so it can help you

<h1>CHECKOUT</h1>
<PanelStyle>
<FormLayout onSubmit={handleSubmit}>
<Label>NAME</Label>
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to use caps in the field names. Name them just with first capital letter, that would be enough

flex-direction: column;
align-items: flex-end;
`;
const Card = ({name, img, type, price, quantity}) =>(
Copy link
Contributor

Choose a reason for hiding this comment

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

should not this Card component come from "./components/Card"; same as in ShoppingCart?

<Wrapper>
<Message>Hello {customerName} your order was successfully placed, Thank You for Shopping with us!!</Message>
<br />
<Message>ORDERED LIST</Message>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: don't use caps text in the UI, this is not a good practice.

{cart.map((item) => (
<Card
key={item.name}
{...item}
Copy link
Contributor

Choose a reason for hiding this comment

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

better would be to not just spread whatever is inside item object, but explicitely pass props one by one. This will increase quality of the code (and you want need to turn off eslint rules)

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