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

2조 과제 제출 #9

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

2조 과제 제출 #9

wants to merge 3 commits into from

Conversation

hookor
Copy link

@hookor hookor commented Jul 2, 2023

Colley

🔗 배포주소

Colley

📄 프로젝트 소개

거래 API를 활용한 온라인 쇼핑몰입니다.

🗓 프로젝트 기간

2023.05.30 ~ 2023.07.02

🧑‍💻 기술 스택

🤝 개발팀 소개

이은비 안중후 문현수 방미선 김동해
프로젝트 팀장,
관리자 페이지,
위시리스트,
마이페이지 - 구매
Github 관리,
구매 페이지, 계좌,
장바구니
로그인, 회원가입,
장바구니,
디자인 가이드,
마이페이지 - 조회,수정
디자인 가이드 메인 페이지,
상품 상세,
상품 카테고리별 페이지

📂 폴더구조

📦src  
┣ 📂api  
┣ 📂assets  
┣ 📂components  
┃ ┣ 📂admin  
┃ ┣ 📂cart  
┃ ┣ 📂common  
┃ ┣ 📂main  
┃ ┣ 📂mypage  
┃ ┣ 📂payment  
┃ ┣ 📂product  
┃ ┣ 📜App.tsx  
┃ ┗ 📜index.ts  
┣ 📂constants  
┣ 📂contexts  
┣ 📂hooks  
┣ 📂pages  
┃ ┣ 📂admin  
┃ ┣ 📂login  
┃ ┣ 📂mypage  
┃ ┣ 📂payment  
┃ ┣ 📂product  
┃ ┣ 📜Home.tsx  
┃ ┣ 📜Router.tsx  
┃ ┗ 📜index.ts  
┣ 📂services  
┣ 📂styles  
┃ ┣ 📂abstracts  
┃ ┣ 📂components  
┃ ┃ ┣ 📂admin  
┃ ┃ ┣ 📂cart  
┃ ┃ ┣ 📂main  
┃ ┃ ┣ 📂mypage  
┃ ┃ ┣ 📂payment  
┃ ┃ ┣ 📂product  
┃ ┃ ┣ 📜Modal.module.scss  
┃ ┃ ┣ 📜badge.module.scss  
┃ ┃ ┗ 📜errorComponent.module.scss  
┃ ┣ 📂layout  
┃ ┣ 📂pages  
┃ ┗ 📜common.scss  
┣ 📂types  
┃ ┣ 📂swiper  
┣ 📂utils  
┣ 📜main.tsx  
┗ 📜vite-env.d.ts

@hookor hookor self-assigned this Jul 2, 2023
Copy link

@sangheon-kim sangheon-kim left a comment

Choose a reason for hiding this comment

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


// 관리자 - 상품 조회
export const adminFetchProducts = async () => {
const response = await adminInstance.get('/products')

Choose a reason for hiding this comment

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

IMP: 전반적으로 api 응답 에러 처리가 되어 있지 않고 있어요 try catch문을 사용해보시는 걸 추천드려요!


//선택 가능한 은행 목록 조회
export const getBankLists = async () => {
const res = await authInstance.get('/account/banks')

Choose a reason for hiding this comment

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

nit: 여기도 try catch 문 사용하시면 에러 처리되고 좋아요

const customerGrade = useMemo(() => {
// 회원등급 (누적금액)
// 브론즈(0~10만) > 실버(10만~20만) > 골드(20만~50만) > 플래티넘(50만~100만) > 다이아몬드(100만~)
if (totalTransactionPrice < 100000) {

Choose a reason for hiding this comment

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

nit: if문 분기 처리보다 switch를 사용하시거나 따로 모듈로 빼셔도 좋을 것 같아요

@@ -0,0 +1,18 @@
import styled from 'styles/components/admin/adminSkeleton.module.scss'

Choose a reason for hiding this comment

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

nit: skeleton은 따로 폴더 만들어서 import 해서 사용하시면 폴더링이 좀더 깔끔해지고 좋아요

<ul>
<div className={styles.notice}>장바구니 이용안내</div>
<li>
해외배송 상품과 국내배송 상품은 함께 결제하실 수 없으니 장바구니

Choose a reason for hiding this comment

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

IMP: 하드코딩 하시는 것보다 json으로 목업 데이터 만들어서 바인딩 하시는 방법도 추천드려요

export const PaymentMethods = () => {
// USER INFO
const { phoneNumber } = useContext(PhoneNumberContext)
const { bank } = useContext(BankContext)

Choose a reason for hiding this comment

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

IMP: paymentInfo라는 상태 하나로 추상화 하면 코드가 훨씬 간결해 지지 않을까요?

import styled from 'styles/components/mypage/myWishItem.module.scss'
import { Link } from 'react-router-dom'
import { WishListContext } from 'contexts/index'
import noImage from 'public/no-photo.png'

Choose a reason for hiding this comment

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

nit: public 생략하고 절대경로로 작성가능해요

}) => {
const navigate = useNavigate()
const { setUserCart } = useContext(CartContext)
const { address } = useContext(UserAddressContext)

Choose a reason for hiding this comment

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

IMP: 유저 관련 상태값을 하나의 각각 개별 context로 담은 이유가 있을까요? 컴포넌트 상태값이 여러개이고 동시에 바껴야 하는 경우 하나의 객체에 담아서 관리하면 코드가 더 깔끔해질 수 있어요


export const DaumPostCode = () => {
const { address, setAddress } = useContext(UserAddressContext)
const { setAddressDetail } = useContext(AddressDetailContext)

Choose a reason for hiding this comment

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

nit: 이부분도 하나의 콘텍스트에 담아서 관리하는 걸 추천 드려요

// 로그인 X + guest 장바구니 초기화
localStorage.setItem(`cart-guest`, JSON.stringify([]))
} else {
if (guestItems !== null && guestItems.length > 0) {

Choose a reason for hiding this comment

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

nit: 중첩 조건문으로 인해 useEffect 콜백함수가 무거워 보여요 가능하면 따로 모듈로 빼서 관리하는게 좋아요

Copy link

@sangheon-kim sangheon-kim left a comment

Choose a reason for hiding this comment

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

</div>
) : null}

{isModalShow && modalProps ? (

Choose a reason for hiding this comment

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

modal을 props로 전달하지 않고, 하는방법은 없을까요? 매번 모달 프롭스를 전달하는방식보다 전역에서 선언해두고 상태로 관리하는게 좋을 것 같아요

<AdminSalesSkeleton />
<AdminSalesSkeleton />
<AdminSalesSkeleton />
<AdminSalesSkeleton />

Choose a reason for hiding this comment

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

그냥 Array.fill 채우시고 하시는게 더 편안해보이는데여... 매번 복사를 하실필요가 있을까 생각들어요

export const Cart = () => {
return (
<>
<div className={styles.wrapper}>

Choose a reason for hiding this comment

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

Fragment를 왜씌운걸까요?

/>
</div>
) : (
<div></div>

Choose a reason for hiding this comment

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

null로 처리해줘도되지않을까요? 또는 && 로 해도 될 것 같아요

/>
</div>
) : (
<div></div>

Choose a reason for hiding this comment

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

삼항연산자로 고집하신 이유가있을까요?

<div className={styled.container}>
<div className={styled.path}>
<Link to="/">홈</Link>
<span>/</span>

Choose a reason for hiding this comment

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

&:after로 처리해도될 것 같은데여

<UserAddressContext.Provider value={{ address, setAddress }}>
<BankContext.Provider value={{ bank, setBank }}>
<PhoneNumberContext.Provider value={{ phoneNumber, setPhoneNumber }}>
<AccountNumberContext.Provider

Choose a reason for hiding this comment

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

Payment Provider를 하나만들어서 관리하는게 나중에 유지보수가 올라갈것 같아요 nested가 너무 depth가 깊어서 보기안좋아요

Copy link

@sangheon-kim sangheon-kim left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 부족한 부분 코멘트 달았어요

추가로, type선언한것에 그 어느것도 jsdoc으로 설명이 안달려있네요 유지보수 하기 어려울 것 같아요 그리고 declaration으로 선언하면 굳이 매번 import로 받을 필요 없을 것 같아요

<div className="product-sort">
<ul>
<li>
<a

Choose a reason for hiding this comment

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

하드코딩 안하면, 불필요하게 중복 코딩 안나올것 같아요

@@ -0,0 +1,41 @@
export interface Address {

Choose a reason for hiding this comment

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

jsdoc으로 설명 달아주면 좋을 것 같아요

@@ -0,0 +1,6 @@
export type AdminDashboardCardProps = {

Choose a reason for hiding this comment

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

declaration type을 사용하지 않고, export를 사용하신이유가있나요?

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