-
Notifications
You must be signed in to change notification settings - Fork 14
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
[임동민] Good-Night-3rd-Hackathon-Frontend 제출 #1
base: main
Are you sure you want to change the base?
Conversation
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.
동민님 안녕하세요.
해커톤 참여하시느라 너무 고생이 많으셨습니다~~
UI 부분을 포함해 코드를 보면서 신경쓰신 부분이 많은 것 같다고 느꼈어요.
최대한 도움을 드리고 싶어 리뷰를 남기긴했지만, Next는 실무에서 사용해본경험이 적어 프레임워크 및 SSR에 대한 부분은 다소 제외하고 의견을 남겼습니다.
어제 우연찮게 장동현님이랑 같이 코드를 구경하면서 주신 의견이 있는데 리뷰 내용에 전부 포함시켜두었습니다.
리뷰를 진행하면서 저도 많이 배워갈 수 있었던 것 같아요. 고생많으셨고 리뷰 관련해서 의견이나 질문있으시다면 꼬리질문으로 편하게 말씀부탁드립니다.
router.push('/wish-fruit/allow'); | ||
}; | ||
return ( | ||
<button |
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.
컴포넌트 하위에 속한 모든 버튼 컴포넌트에 대한 전반적인 의견 남겨드립니다.
폴더 구조상 범용 컴포넌트를 의도하신 것 같은데, 컴포넌트에 도메인 로직이 섞여있는 요소가 몇몇 보입니다.
오히려 "수행할 기능" , "UI" 부분을 props로 받아 �처리하는 버튼 컴포넌트 하나를 만들어서 이를 재활용하는게 더 괜찮았을 것 같아요.
버튼 내 이미지의 경우도 props로 받아서 유연하게 처리할 수 있도록 구현했다면 굳이 컴포넌트가 세분화되지 않아도 되었을 것 같다고 생각합니다.
개인적인 팁인데 이름 자체에 측정 도메인 로직이 섞여들어갔는지 확인하면 순수 컴포넌트의 기능을 잘 수행하고 있는지 확인하기 좋은 것 같습니다. 이 경우 WishFruit, WishSection
가 반례로 있겠네요.
추가로 React-query는 시간적인 문제로 사용하지 않으신건지 궁금합니다. 예를 들어 코드내에서 "Comment를 달면 api를 다시 호출하는 부분"과 같이 api와의 의존관계를 직접 처리하지 않고 Query에서 관리하면 코드가 깔끔해지고 서버데이터에 대한 관심사를 분리할 수 있어서 좋은점이 많거든요. |
리뷰 확인했습니다. |
Preview
Aug-25-2024.22-32-52.mp4
Next로 작업하였습니다.
추가기능 포함하여 모든 기능구현 완료하였습니다.