-
Notifications
You must be signed in to change notification settings - Fork 39
React 염휘건 sprint6 #187
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
React 염휘건 sprint6 #187
The head ref may contain hidden characters: "React-\uC5FC\uD718\uAC74-sprint6"
Conversation
addiescode-sj
left a comment
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.
수고하셨습니다!
주요 리뷰 포인트
- 복잡도 감소를 위한 컴포넌트 분리
- 예외 케이스 처리하기
| // todo: /additem 으로 이동 | ||
| // /additem 으로 이동 | ||
| navigate('/additem'); | ||
| console.log('상품 추가 버튼 클릭'); |
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.
불필요한 로그는 PR 올리실때 지워봅시다! :)
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.
BEM 방식으로 잘 nesting 되어있네요. 굳굳 👍
| }; | ||
| const removeTag = (t: string) => |
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.
55, 56 사이 공백 한칸 띄워줄까요? 함수와 함수 사이엔 공백 한칸씩 꼭 띄우도록합시다 :)
| const value = tagInput.trim(); | ||
| if (!value || tags.includes(value)) return; | ||
|
|
||
| setTags((prev) => [...prev, value]); |
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.
spread 연산자를 사용해 배열을 복사 후 새 아이템을 추가하는군요!
Array또한 레퍼런스 타입이라 array.push로 직접적으로 변경하지않고 지금과 같이 원본 배열을 변경하지않는 방식이 좋습니다 :)
이 과정에서 배열을 다룰때 발생할 수 있는 예외 케이스 (예를 들어, 중복값을 허용할것인지 등) 를 꼼꼼하게 더 처리해볼까요?
| const handleSubmit = () => { | ||
| console.log({ images, name, description, price: Number(price), tags }); | ||
| }; |
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.
해당 함수는 실제 제출을 담당하지는 않고 로그만 찍는 용도인가요? :)
| <input | ||
| className={style['product-add-page__input']} | ||
| placeholder="상품명을 입력해주세요" | ||
| value={name} | ||
| onChange={(e) => setName(e.target.value)} | ||
| /> |
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.
ProductAddPage의 복잡도를 감소시키기위해,
폼 컨트롤 + 업데이트 로직을 다른 컴포넌트 단위로 분리해보는건 어떨까요?
Form, FormInput 두개의 컴포넌트를 사용해 리팩토링해봐요!
요구사항
기본
상품 이미지는 최대 한개 업로드가 가능합니다.
-[x]이미지를 제외하고 input 에 모든 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.
심화
주요 변경사항
스크린샷