-
Notifications
You must be signed in to change notification settings - Fork 37
[박인건] sprint6 #259
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
[박인건] sprint6 #259
The head ref may contain hidden characters: "React-\uBC15\uC778\uAC74-sprint6"
Conversation
1005hoon
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.
인건님, 이번 프로젝트도 고생 많으셨습니다!
뭔가 구조가 많이 깔끔해졌어요. 최대한 리뷰사항 반영해서 작업하시려 하는 모습도 보입니다.
쉽지 않으실텐데도 노력해주셔서 감사해요.
이제 조금 더 욕심 내볼만한건,
- 코딩 컨벤션: 변수 명 / 코드 작성 스타일
- 에러 핸들링
- 폼 관리
정도가 될텐데요,
코딩 컨벤션의 경우 코드에 남겨둔 리뷰 한번 체크해주시면 될것 같구
에러 핸들링의 경우, 다음 콘텐츠 한번 살펴보시구, 다른 포스트들 조금 참고해서 어떻게 처리하는게 좋을지 고민하는 시간을 한번 가져보시면 도움이 될 듯 해요.
그리고 폼 관리는 프론트엔드에서 가장 어려운 영역인데요, 그런만큼 효율적으로 관리하는 방법에 대한 리서치가 많이 진행되어왔습니다. 한번 이 내용들도 탐색해보시고 적용해보면 어떨까요?
이번 프로젝트도 고생 많으셨습니다
| @@ -0,0 +1,18 @@ | |||
| import { baseURL } from "../constants/VariableSetting"; | |||
|
|
|||
| export async function CreateCommentAPI({ productId, accessToekn }) { | |||
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.
Class와 Component가 아닌 이상 함수 또는 변수 이름을 대무낮로 시작하지 말아주세요!
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.
음 조금 고민이 되는게 access token 을 함수에 전달해주는게 과연 올바른 방법일까 싶네요.
access token을 관리하는 전역 상태가 하나가 있는 상황에서, 이 함수 내부에서 토큰을 조회할 수 있도록 작업하는게 좀 더 좋겠다는 생각이 문득 들어요
| export async function CreateCommentAPI({ productId, accessToekn }) { | ||
| const response = await fetch(`${baseURL}/products/${productId}/comments`, { | ||
| method: "POST", | ||
| body: JSON.stringify({}), |
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.
댓글 생성을 위해 body가 필요할텐데요, 이 부분도 추가되면 좋겠습니다
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error("상품 정보를 가져오는데 실패했습니다."); |
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.
상품 정보가 아니라 댓글 생성인거죵?
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.
또한 이렇게 처리가 된다면 서버에서 디버깅 목적으로 제공해준 오류 response를 받을 수 없답니다
| page = 1, | ||
| pageSize = 10, | ||
| orderBy = "recent", | ||
| keyword = "", |
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.
default value처리 너무 좋습니다
| @@ -0,0 +1,17 @@ | |||
| import { baseURL } from "../constants/VariableSetting"; | |||
|
|
|||
| export async function SginInAPI({ productId, ID, Password }) { | |||
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.
오타가 있어보입니다,
그리구 ID와 Password를 대문자로 받아야 할 필요는 없어보여ㅛ!
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error("상품 정보를 가져오는데 실패했습니다."); |
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.
에러 로깅 신경써주세요!
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.
Button_Module 에서 모듈이라는 이름이 들어갈 필요가 있을까요?
| const [productName, setProductName] = useState(""); | ||
| const [productInfo, setProductInfo] = useState(""); | ||
| const [productPrice, setProductPrice] = useState(""); | ||
| const [productTag, setProductTag] = useState(""); | ||
| const [preview, setPreview] = useState(null); | ||
| const [hashTags, setHashTags] = useState([]); |
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.
좀 더 이 폼 상태를 유려하게 관리하는 방법이 있을 것 같아요!
지금 형태는 필드 하나가 늘어날때마다 이를 관리하기 위한 상태와, 이벤트 처리를 위한 함수 두개씩 생기고 있는데
scalable하지 않아보입니다
| const [allProducts, setAllProducts] = useState([]); | ||
| const [orderBy, setOrderBy] = useState(); | ||
| const [keyword, setKeyword] = useState(); | ||
| const imageSize = "220px"; |
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.
디폴트 이미지 사이즈를 관리하는건 container 목적의 콤포넌트보단 ui presenter 목적의 콤포넌트에서 관리하는게 더 좋지 않을까 생각되어요!
| const data = await GetProductsAPI({ | ||
| page: 1, | ||
| pageSize: 10, | ||
| orderBy: orderBy, | ||
| keyword: keyword, | ||
| }); | ||
| // console.log(data); | ||
| setAllProducts(data); | ||
| } catch (error) { | ||
| throw new Error("전체 상품 로드 실패:", error); | ||
| } |
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.
음 catch문에서 지속적으로 에러를 던지고 있는데,
api.js에서 던진 에러는 여기서 잡아준다 쳐도,
여기서 에러를 던져버리면 서버 오류 발생시 앱이 터져버리게 됩니다.
어디서 에러를 던지고, 어디서 캐치를 해올지 조금 고민이 필요해보여요
요구사항
기본
-[] 상품 이미지는 최대 한개 업로드가 가능합니다.
심화
주요 변경사항
스크린샷
멘토에게