Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[이영훈] sprint7 #718

Conversation

tkddbs587
Copy link
Collaborator

@tkddbs587 tkddbs587 commented Jul 12, 2024

요구사항

기본

  • 각 상품 클릭 시 상품 상세 페이지로 이동합니다.
  • 상품 상세 페이지 주소는 “/items/{productId}” 입니다.
  • response 로 받은 아래의 데이터로 화면을 구현합니다
  • 목록으로 돌아가기 버튼을 클릭하면 중고마켓 페이지 주소인 “/items” 으로 이동합니다
  • 문의하기에 내용을 입력하면 등록 버튼의 색상은 “3692FF”로 변합니다.
  • response 로 받은 아래의 데이터로 화면을 구현합니다

멘토에게

  • 스프린트 7 미션을 빼먹었어서 이번 기회에 올려요 멘토님!!
  • Next로 구현하느라 Next.js-이영훈-sprint10 브랜치에 이어서 작업했습니다
  • UI(반응형), 댓글 등록, 요구사항엔 없지만 API에 patch랑 delete도 있어서 댓글 수정, 삭제 기능까지 구현했습니다!

스크린샷

상품 상세 페이지 스크린샷 2024-07-12 오후 10 46 36
상품 상세 페이지 댓글 스크린샷 2024-07-12 오후 10 47 54

@tkddbs587 tkddbs587 requested a review from jyh0521 July 12, 2024 13:50
@tkddbs587 tkddbs587 added enhancement New feature or request 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Jul 12, 2024
@tkddbs587 tkddbs587 removed the 미완성🫠 죄송합니다.. label Jul 12, 2024
Copy link
Collaborator

@jyh0521 jyh0521 left a comment

Choose a reason for hiding this comment

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

과제하느라 고생하셨습니다! 잘 만들어주셨네요 👍

@@ -8,26 +11,25 @@ export async function getArticlesData({
keyword = "",
}) {
const query = `page=${page}&pageSize=${pageSize}&orderBy=${orderBy}&keyword=${keyword}`;
const res = await fetch(`${BASE_URL}/articles?${query}`);
const data = await res.json();
const res = await axios.get(`/articles?${query}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

fetch에서 axios 활용해보신 점 좋네요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

api 함수를 각 파일로 분리해서 작성하실때는 해당 함수 명을 파일로 이름을 짓기도 합니다. 파일 같은 경우는 getProductsData로 지어주는 것이죠!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 참고하겠습니다!

}) => {
const [kebabButton, setKebabButton] = useState(false);
const [kebabButtonVisible, setKebabButtonVisible] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

상태 이름을 더 명시적으로 수정해주신 점 좋네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

멘토님한테 배운대로 적용해봤습니다! 🫡

Comment on lines +29 to +32
if (comment.id === targetId) {
return data;
}
return comment;
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 로직은 삼항연산자로 대체 해주셔도 좋습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵! 수정해보겠습니다


useEffect(() => {
async function loadData() {
const data = await getArticlesData({
page: page,
Copy link
Collaborator

Choose a reason for hiding this comment

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

객체에서 필드명이랑 변수명이 같으면 한번에 작성 가능한 것 알고 계시죠? ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 알고 있습니다 수정하겠습니다!

Comment on lines 32 to 34
const handleInputChange = (
e: ChangeEvent<HTMLTextAreaElement | HTMLInputElement>
e: ChangeEvent<HTMLTextAreaElement | HTMLInputElement>,
) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 함수도 저번에 textarea와 input을 분리했던 것처럼 똑같이 분리해서 사용해주셔도 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 참고하겠습니다 !

const [search, setSearch] = useState("");
const [totalPage, setTotalPage] = useState(1);
const [page, setPage] = useState(1);
const [order, setOrder] = useState("recent");
Copy link
Collaborator

Choose a reason for hiding this comment

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

recent 상수로 반영해주시면 좋을 것 같아요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

컬러변수 활용 누락된 부분 반영해주시면 좋을 것 같습니다.

Comment on lines +12 to +13
setOrder: React.Dispatch<React.SetStateAction<string>>;
setSearch: React.Dispatch<React.SetStateAction<string>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 경우에 setState 함수를 감싸는 handleInputChange 함수 자체를 외부에서 받아오게 사용하시는 것도 좋습니다. setSearch를 사용하는 것 이외의 로직이 추가되야 한다면, 현재 상태에서는 구현하기 어려워 보이기 때문입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 그러면 handleInputChange 함수 자체를 상위 컴포넌트에서 만들고 prop으로 내려주는게 더 좋다는 말씀이신거죠?!

body {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
font-family:
Copy link
Collaborator

Choose a reason for hiding this comment

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

폰트가 없을때를 대비해서 여러개를 준비해주신 점 좋네요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 글로벌 스타일 설정 해주면 좋더라구요 코드 리뷰 감사합니다 멘토님!!

@jyh0521 jyh0521 merged commit fad1405 into codeit-bootcamp-frontend:Next.js-이영훈 Jul 14, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants