-
Notifications
You must be signed in to change notification settings - Fork 31
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
[김홍섭] Sprint5 #113
The head ref may contain hidden characters: "basic-\uAE40\uD64D\uC12D"
[김홍섭] Sprint5 #113
Conversation
eslint.config.js
Outdated
import reactRefresh from 'eslint-plugin-react-refresh' | ||
|
||
export default [ | ||
{ ignores: ['dist'] }, |
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.
eslint 버전 9에 맞게 잘 작성하셨어요. eslint 8 버전까지는 .eslintignore
로 따로 분리했는데 9부터는 config.js
에 통합되었습니다:)
src/services/ArticleService.js
Outdated
|
||
// 핸들러 | ||
const requestErrorHandler = (e) => { | ||
if (e.response) { |
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.
에러 코드별로 세분화해보면 어떨까요? 400번대 에러일 때랑 500번대 에러일 때 메시지가 다르면 좋을 것 같아요!
src/services/ArticleService.js
Outdated
return `처리 중 오류가 발생했습니다. 다른 값을 입력해보세요.`; | ||
} else { | ||
return `서버에 접속하지 못했습니다.`; | ||
} |
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.
이미 분기문에서 return을 했으니 굳이 else를 쓰시지 마시고 바로 디폴트 에러 메시지를 리턴하시는게 더 좋습니다!
src/services/ArticleService.js
Outdated
createArticle, | ||
deleteArticle, | ||
patchArticle, | ||
}; |
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.
요구사항에는 axios가 아닌 fetch api를 사용해서 구현해라고 나와있는데, axios를 선택하신 이유가 있으신가요?
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.
ArticleService
에서 요구사항에서는 async, await 사용보다는 Promise의 개념의 이해를 위해 async-await 사용이 아니라 return fetch(...).then...
이런 식으로 바로 promise를 리턴하는 형태를 제시한 것 같습니다. 나중에 useEffect 안에서 api 호출 시 해당 개념을 알아야 하거든요.
하지만 동작 원리는 같아요! async-await을 사용하는 이유가 더 편한 가독성을 위한거니깐요! 실무에서도 이렇게 더 많이 쓰긴 합니다.
src/services/ProductService.js
Outdated
createProduct, | ||
deleteProduct, | ||
patchProduct, | ||
}; |
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.
https://panda-market-api-crud.vercel.app
https://panda-market-api-crud.vercel.app/products
https://panda-market-api-crud.vercel.app/articles
는 별도의 변수로 빼는 것이 더 좋을것 같아요!
src/services/ArticleService.js
Outdated
createArticle, | ||
deleteArticle, | ||
patchArticle, | ||
}; |
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.
ArticleService
에서 요구사항에서는 async, await 사용보다는 Promise의 개념의 이해를 위해 async-await 사용이 아니라 return fetch(...).then...
이런 식으로 바로 promise를 리턴하는 형태를 제시한 것 같습니다. 나중에 useEffect 안에서 api 호출 시 해당 개념을 알아야 하거든요.
하지만 동작 원리는 같아요! async-await을 사용하는 이유가 더 편한 가독성을 위한거니깐요! 실무에서도 이렇게 더 많이 쓰긴 합니다.
src/services/ProductService.js
Outdated
} else { | ||
const res = await axios.get( | ||
`https://panda-market-api-crud.vercel.app/products?page=${page}&pageSize=${pageSize}&orderBy=${orderBy}` | ||
); |
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.
이 부분 3항연산자 쓰셔서 조금 더 줄이시면 더 좋을것 같아요~
src/main.jsx
Outdated
<StrictMode> | ||
<App /> | ||
</StrictMode>, | ||
) |
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.
요구사항에서는 엔트리 포인트로 React가 아니라 ArticleService
와 ProductService
를 불러오는것으로 보입니다ㅠ
src/services/ArticleService.js
Outdated
|
||
// 게시글목록조회 | ||
const getArticleList = async (page, pageSize, orderBy = "recent", keyword) => { | ||
try { |
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.
page, pageSize 여기서 default 값을 처리해주고, 만일 숫자가 아닌 경우 체크해서 예외 처리를 해주시는게 좋을거같아요.,
commit 단위를 작업별로 잘게 쪼개시면 좋을 것 같아요.
|
src/services/ProductService.js
Outdated
|
||
// 상품생성 | ||
const createProduct = async (createProductData) => { | ||
try { |
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.
productData에 들어가야 하는 항목인 name, description, price, manufecturer, tags, images
를 검증하는 로직 추가해주세요.
src/services/ProductService.js
Outdated
|
||
// 상품수정 | ||
const patchProduct = async (productId, patchProductData) => { | ||
try { |
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.
여기도 마찬가지에요! 공통으로 검증 로직 빼주시면 더 좋습니다.
src/services/ArticleService.js
Outdated
`https://panda-market-api-crud.vercel.app/articles?page=${page}&pageSize=${pageSize}&orderBy=${orderBy}` | ||
); | ||
return res.data; | ||
} |
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문 쓰지 않고 한번 짜보세요!
src/services/ArticleService.js
Outdated
|
||
// 게시글생성 | ||
const createArticle = async (createArticleData) => { | ||
try { |
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.
createArticleData 에 들어가야 하는 항목인 name, description, price, manufecturer, tags, images
를 검증하는 로직 추가해주세요.
src/services/ArticleService.js
Outdated
}; | ||
|
||
// 게시글수정 | ||
const patchArticle = async (articleId, patchArticleData) => { |
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.
여기도 두 번째 매개변수 값 검증 로직 추가해주세요! 공통으로 빼시면 더 좋습니다.
src/services/ProductService.js
Outdated
// 핸들러 | ||
const requestErrorHandler = (e) => { | ||
if (e.response) { | ||
return `처리 중 오류가 발생했습니다. 다른 값을 입력해보세요.`; |
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 에러 상태코드도 같이 출력해주세요.
피드백 감사드립니다. 이번 PR에서 배포 URL을 추가하지 않았는데, 다음 PR때는 배포 URL도 함께 제출하겠습니다. |
frontend/public/static/faq.html
Outdated
<body> | ||
<h1>임시 FAQ 페이지</h1> | ||
</body> | ||
</html> |
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.
이 부분은 왜 SPA 구조로 안 짜신건가요?
} | ||
}); | ||
} | ||
}); |
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.
Vanilla.js가 아니라 React 입니다.
{ email: "[email protected]", password: "codeit404!" }, | ||
{ email: "[email protected]", password: "codeit505!" }, | ||
{ email: "[email protected]", password: "codeit606!" }, | ||
]; |
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.
mock data를 다음처럼 분리는 잘 하셨습니다!
// TODO: 사용 안함 | ||
signupSuccess: "회원가입이 성공적으로 처리되었습니다.", | ||
// loginError: "존재하지 않는 이메일, 혹은 비밀번호입니다.", | ||
}; |
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.
이렇게 mapping 하신건 잘 하셨습니다. typescript로는 Record<string, 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.
디자인 적용 누락되신거 아시죠ㅠㅠ
const productListResponse = await getProductList(1, 4, 'favorite'); | ||
setProductList(productListResponse); | ||
} catch (err) { | ||
console.error(err); |
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.
에러 처리는 잘 하셨습니다.
useEffect(() => { | ||
const fetchProductList = async () => { | ||
try { | ||
const productListResponse = await getProductList(1, 4, 'favorite'); |
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.
페이지네이션을 context api를 써서 custom hook을 쓰시는 방법 고민해보세요!
@@ -0,0 +1,87 @@ | |||
import axios from "axios"; | |||
|
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.
***service.js로 미션4 파일 그대로 두신 이유가 있을까요?
요구사항
기본
https://panda-market-api.vercel.app/docs/
에 명세된 GET 메소드 '/products'를 사용했습니다.심화
주요 변경사항
Vite + React + Js
를 사용했습니다.배포 URL
https://6-sprint-misson-fe-dev-pr5.vercel.app/
스크린샷
멘토에게