-
Notifications
You must be signed in to change notification settings - Fork 1
E-commerce App with Tensei #10
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
base: master
Are you sure you want to change the base?
Conversation
e-commerce/client/src/App.tsx
Outdated
import ProductCard from './components/ProductCard'; | ||
import Cart from './components/Cart'; | ||
|
||
import { useQuery } from 'react-query'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here's what we're gonna do. We're going to use graphql-codegen
to generate types for the queries. You'll need to learn how to create query files, and configure graphql codegen for react-query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
e-commerce/client/src/App.tsx
Outdated
amount: number | ||
} | ||
|
||
const endpoint = 'http://localhost:8810/graphql' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd have to start using environment variables so we can deploy to the cloud
e-commerce/client/src/App.tsx
Outdated
} | ||
` | ||
|
||
const fetchProducts = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you generate types with react-query and graphql-codegen, this function will be handled. Let me know how your research on how to use it goes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
e-commerce/client/src/App.tsx
Outdated
|
||
const handleRemoveFromCart = (id: number) => { | ||
setCartLists(prevItem => ( | ||
prevItem.reduce((ack, item) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use better variable names. I'm not sure what ack
is. also, prevItem
might be clearer as previousItem
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
import { Product } from '../App' | ||
|
||
type CartProp = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CartProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
handleRemoveFromCart: (id: number) => void | ||
} | ||
|
||
const Cart:React.FunctionComponent<CartProp> = ({ cartLists, handleAddToCart, handleRemoveFromCart }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not cartLists
, its not a list, but rather a list of items, so cartItems
would be better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
handleRemoveFromCart: (id: number) => void | ||
} | ||
|
||
const Cart:React.FunctionComponent<CartProp> = ({ cartLists, handleAddToCart, handleRemoveFromCart }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead of calling your props handleAddToCart
or handleRemoveFromCart
, its best to call them onAddToCart
, onRemoveFromCart
. Much better because all event listeners in react start with on
and not handle
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
const ProductCard:React.FunctionComponent<ProductCardProp> = ({ product, handleAddToCart }) => { | ||
return ( | ||
<div className="column is-half"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting is off I think. Do you have prettier setup ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did a prettier setup and worked on the formatting. It looks much better.
e-commerce/client/src/App.tsx
Outdated
}; | ||
|
||
const onRemoveFromCart = (id: number) => { | ||
setCartItems((findProductItem) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is not readable. Try breaking it down into multiple steps. So its readable and understandable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
@@ -0,0 +1,10 @@ | |||
query Products { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this automatically generated? Shouldn't be here I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requests in this file was used to auto-generate types by graphql codegen.
|
||
type ProductCardProps = { | ||
product: Product; | ||
onAddToCart: (clickedProduct: Product) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A much better variable name to clickedProduct
is product
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
}; | ||
|
||
const Navbar: React.FunctionComponent<NavbarProp> = ({ cartItems }) => { | ||
const getCartTotal = (products: CartItem[]) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable name should be cartItems: CartItem[]
. Not products: CartItem[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
const App = () => { | ||
const [cartItems, setCartItems] = useState<CartItem[]>([]); | ||
const { data, isLoading, error } = useProductsQuery<ProductsQuery>({ | ||
endpoint: "http://127.0.0.1:8810/graphql", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try searching for a way to not have this here ? I'm thinking there should be a way to globally define this in the fetcher used by the generated products.
e-commerce/api/server.ts
Outdated
} | ||
}) | ||
|
||
const totalPrice = products.reduce((total:any, product:any) => total + parseInt(product.price), 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not use any
here. total is number
and product is ProductModel
. You can import ProductModel
from '@tensei/orm`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
import { graphql } from '@tensei/graphql' | ||
import { welcome, tensei, cors, resource, text, textarea, integer, graphQlQuery } from '@tensei/core' | ||
|
||
const stripe = new Stripe('sk_test_51JjsxtJnZnb1n2GnbebKQgYe73xTZtT1147z1dLFU9CGmkfbZKvcrW1VX4jA10Wav82slFIGjp9AW6puPURjibMP00z7Fw14Em', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use process.env.STRIPE_SECRET
here. The article should note that this key will be set in environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
e-commerce/client/src/App.tsx
Outdated
import Cart from "./components/Cart"; | ||
import { Checkout } from "./components/Checkout"; | ||
import PaymentSuccessful from "./components/PaymentSuccessful"; | ||
import { isTemplateTail } from "typescript"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, it was not meant to be there.
Done.
e-commerce/api/server.ts
Outdated
.fields([ | ||
text('Image'), | ||
text('Name'), | ||
text('Price'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Price should actually be a number (integer) field.
const getTotal = (products: CartItem[]) => | ||
products.reduce( | ||
(accumulator: number, product) => | ||
accumulator + product.quantity * Number(product.product.price), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change the price to a number field, you won't need to use Number(
here.
// if succedded, redirect to thank you page | ||
setError(null); | ||
setProcessing(false); | ||
setSucceeded(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need success and redirect. Here, we can just call router.push
i think to redirect the user. Please let me know if this function exists. There should be a hook or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
No description provided.