-
Notifications
You must be signed in to change notification settings - Fork 39
[명지우] sprint7 #210
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
[명지우] sprint7 #210
The head ref may contain hidden characters: "React-\uBA85\uC9C0\uC6B0-sprint7"
Conversation
GANGYIKIM
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.
지우님 7번째 미션 작업 고생하셨습니다!
리뷰가 진행 중일 때 새로운 커밋이 추가되면 변경 내역이 꼬이거나 리뷰가 누락될 수 있어요.
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.
💊 제안
파일명이 다른 파일명과 같은 네이밍 룰에 따라 명명되면 좋겠네요~
| let url = `/products/${productId}/comments?limit=${limit}`; | ||
| if (cursor) { | ||
| url += `&cursor=${cursor}`; | ||
| } |
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.
💊 제안
getProductComments에서 쿼리 문자열을 직접 조합하기보다는 URLSearchParams을 활용하시면 가독성과 유지보수성이 더 좋을 것 같아요~
https://developer.mozilla.org/ko/docs/Web/API/URLSearchParams
| const [isOpen, setIsOpen] = useState(false); | ||
|
|
||
| const handleToggle = () => setIsOpen((prev) => !prev); |
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.
💬 여담
reducer 를 사용해 아래처럼 간단하게도 작성 가능합니다~
| const [isOpen, setIsOpen] = useState(false); | |
| const handleToggle = () => setIsOpen((prev) => !prev); | |
| const [isOpen, toggleIsOpen] = useReducer((prev) => !prev, false); |
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 () => { | ||
| if (target) observer.unobserve(target); | ||
| }; |
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.
💊 제안
unobserve보다 disconnect() 메서드로 observer 자체를 종료하는 것이 더 적절합니다.
| return () => { | |
| if (target) observer.unobserve(target); | |
| }; | |
| return () => observer.disconnect(); |
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 DropdownMenu = ({ | ||
| dropdownItem1, | ||
| onDropdownItem1Click, | ||
| dropdownItem2, | ||
| onDropdownItem2Click, | ||
| position = "left", | ||
| }) => { |
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.
💊 제안
지금 컴포넌트 구조상 메뉴가 2개인 경우만 사용이 가능합니다~
공용 컴포넌트인만큼 여러개인 경우도 커버할 수 있게 수정해보시면 더 좋을 것 같아요!
| }, [loadComments]); | ||
|
|
||
| // 마지막 요소를 감지하는 observer | ||
| useObserver(observerRef, loadComments); |
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.
💬 여담
무한스크롤을 구현하려고 하시는 것 같아요. intersection observer를 연결하실 때는 마지막 요소보다 2~3개정도 위의 요소에 연결하셔서 사용자가 스크롤하는데 불편함을 느끼지 않게 하시면 더 좋습니다.
| textarea.style.height = "auto"; | ||
| textarea.style.height = `${textarea.scrollHeight}px`; |
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.
❗️ 수정요청
| textarea.style.height = "auto"; | |
| textarea.style.height = `${textarea.scrollHeight}px`; | |
| textarea.style.height = `${textarea.scrollHeight}px`; |
| <StyledButton | ||
| onClick={onClick} |
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.
❗️ 수정요청
| <StyledButton | |
| onClick={onClick} | |
| <StyledButton | |
| type="button" | |
| onClick={onClick} |
판다마켓 7
🌐 배포 url: https://myungjiwoo-pandamarket.netlify.app/items
기본 요구사항
체크 리스트 (기본)
체크 리스트 (심화)
스크린샷
멘토에게