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

Ecommerce development v1 #1

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

Ecommerce development v1 #1

wants to merge 5 commits into from

Conversation

ritika99
Copy link
Owner

@ritika99 ritika99 commented May 5, 2021

No description provided.

ritika99 added 2 commits May 6, 2021 00:14
feat: show products
add to cart and wishlist
sort and filter by price and categories
responsive design
Comment on lines +13 to +15
<button className="btn btn-primary" onClick={() => navigate("/products")}>
Shop Now
</button>
Copy link

@DanishOnGit DanishOnGit May 5, 2021

Choose a reason for hiding this comment

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

Hey Ritika,
Its better if u wrap the btn inside Link and navigate using that.

<Link to="/products"> <button className="btn btn-primary" >
        Shop Now
      </button></Link>

You can make similar changes in the Login/signup form

Comment on lines 30 to 35
<button
class="btn btn-link"
onClick={() => navigate("/password/forgot")}
>
Forgot Password?
</button>

Choose a reason for hiding this comment

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

Same as below comment for Link on Shop now btn.

import { EmptyCart } from "./emptyCart";

export function Cart() {
const { data } = useData();

Choose a reason for hiding this comment

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

Suggested change
const { data } = useData();
const { data: { cartList } } = useData();

we can destructure data object here for our ease.

<div className="container-inside margin-top container-flex-column">
<img className="img-md" src={emptyCartImage} alt="Empty cart"/>
<h1 className="text-colored">Cart is empty!</h1>
<button className="btn btn-primary" onClick={() => navigate("/products")}>Shop Now</button>

Choose a reason for hiding this comment

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

Here too, its better to use the Link tag .

import "./nav-menu.css";

export function NavMenu() {
const { data, dispatch } = useData();

Choose a reason for hiding this comment

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

Suggested change
const { data, dispatch } = useData();
const { data:{ navToggle }, dispatch } = useData();

Copy link

@MKumaran98 MKumaran98 left a comment

Choose a reason for hiding this comment

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

Hey Ritika, I've suggested some minor changes do check it out

<form className="form-box">
<h2 class="text-centre">Forgot Password</h2>
<div className="input-field">
<input type="email" className="input-text" id="email" required />

Choose a reason for hiding this comment

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

input can be nested inside label and written like
<label>Email:<input/></label>
So that the id can be avoided


.nav .nav-menu {
position: absolute;
height: cal(100vh - 100%);

Choose a reason for hiding this comment

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

its calc() not cal()

<Routes>
<Route path="/" element={<Home />} />
<Route path="products" element={<Products />} />
<Route path="login" element={<Login />} />

Choose a reason for hiding this comment

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

A check can be added here so that a logged in user is not able to access /login

import logo from "../../assets/satik-logo.png";

export function Navigation() {
const { data, dispatch } = useData();

Choose a reason for hiding this comment

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

Suggested change
const { data, dispatch } = useData();
const { data:{ navToggle,wishList,cartList }, dispatch } = useData();

Comment on lines +33 to +35
return productList.sort((a, b) => a.price - b.price);
else if (sortby === "SORT_PRICE_DESC")
return productList.sort((a, b) => b.price - a.price);
Copy link

@DanishOnGit DanishOnGit May 5, 2021

Choose a reason for hiding this comment

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

This will mutate the original productList array. To check its effect, u can try doing a Clear Filter btn. Hence we should apply sort on a new array. Here's a way we can do it:
[...productList].sort((a, b) => a.price - b.price);
Plz correct me if I am missing anything !

Copy link

@DanishOnGit DanishOnGit left a comment

Choose a reason for hiding this comment

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

Hi Ritika,
The folders are structured well. CSS is also cleanly written with good used of variables and classnames.
I have only made minor refactoring suggestions in the code.

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.

3 participants