-
Notifications
You must be signed in to change notification settings - Fork 28
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
[Mission5/김주하] Project_Notion_VanillaJs 과제 #48
base: 4/#5_kimjuha
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.
수고하셨습니다. 우선 너무 한번에 100% 구조화된 코드를 만들기보다는
하나씩 리펙토링하면서 분리를 하면 좋겠습니다.
최대한 로직은 분리해서 작은단위로 하나의 역할만 하는 함수로 뺴서 명시적인 이름을 주어 작성하면 좋을 것 같습니다.
PostEditPage에서의 setState, DocumentList에서의 eventListener 를 우선 작은 단위로 나누어 생각해보시고, 그러면서 하나씩 구조적으로 리펙토링을 해보면 좋겠습니다.
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.
주하님 과제 고생 많으셨습니다.
전체적인 기능은 잘 구현하신 것 같습니다. UX도 신경써주시면 좋은 애플리케이션이 될 것 같습니다!
다만 DocumentList
의 이벤트 리스너는 상위 컴포넌트에서 처리해주시거나 따로 모듈화하시는 게 좋을 것 같습니다! 고생 많으셨습니다 ㅎㅎ
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.
주하님, 노션 클로닝 과제하시느라 정말 수고 많으셨습니다!
코드리뷰 마감일이 얼마 남지 않아서 제 욕심만큼 꼼꼼하게 코드리뷰를 못해드린 점 정말 죄송합니다ㅠㅠ
그리고, 제가 리뷰하고 싶었던 부분들은 이미 지석멘토님과 창욱님께서 말씀해주셔서.. 제 리뷰 수가 너무 적네요...ㅠㅠ
주하님의 코드를 보면서 좋았던 점은, '부드럽게 읽힌다'는 것이었습니다! 이벤트 리스너 부분처럼 코드가 긴 부분도 읽기는 불편했지만 이해하기가 어렵지는 않았던것 같습니다. 주하님께서 코드를 읽기 쉽게 잘 작성하시는 것 같습니다ㅎㅎ
Q. 코드가 너무 구조화되어 있지 않고, 가독성이 안좋습니다.
컴포넌트는 충분히 잘 구조화하셨고, 제 개인적으로는 가독성이 좋았다고 생각합니다! 저는 주하님만큼 컴포넌트에서 View 로직을 분리하지 못했는데, 주하님은 충분히 잘 분리해주셨고, 그 덕분에 코드를 읽기가 훨씬 수월했다고 느껴집니다.
근데 이 Notion 클로닝 과제 자체가 페이지가 하나만 있고, 컴포넌트를 깔끔하게 나누기 조금 까다로운 요구사항들이 있다보니, 주하님 입장에서는 구조화되어 있지 않고, 가독성이 안좋다고 느껴지실수도 있다고 생각합니다. 제 생각에는 이 부분은 데브코스에서 많은 프로젝트들을 경험하면서 내공이 쌓이면 더 나아질 수 있다고 생각합니다!
Q. 많은 고민을 하였지만, 과제 제출 시간과 많은 로직을 한 번에 신경쓰기 힘들어 방대한 양의 코드를 잘 정리하지 못했습니다.
많은 고민을 하셨던 흔적들이 보입니다! 소피아 매니저님의 말씀대로, 이번에 노션 클로닝 과제를 제출하고 나서 다시는 이 과제에 손을 안댈게 아니잖아요? 과제 이외에도 저희 함께 노션 클로닝 과제를 리팩토링하고 개선해 나가면서 방대한 양의 코드를 정리하는 방법을 훈련하면 된다고 생각합니다ㅎㅎ
Q. 코드 구조화에 대해 피드백 주시면 감사하겠습니다!
저는 개인적으로 Page 는 화면 하나를 가리키는 말이라고 생각해서, 사이드바와 에디터 부분을 Page 라는 이름으로 나눈 부분이 조금 아쉬웠던것 같습니다
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.
주하님 고생하셨습니다~
Document 삭제 처리를 사용자 입장에서 생각해서 구현하신 게 인상 깊습니다! 백엔드가 주는 대로 기능 구현했던 저를 반성하게 되네요. 😌
이건 어디에 남길지 애매해서 여기에 쓰는데 DocumentList 쪽 버그가 몇 개 있습니다.
document 하나를 클릭한 다음에 바로 삭제 버튼을 클릭하면 ul이 두 개가 됩니다. 그리고 Add a Page 버튼을 클릭할 때 종종 렌더링이 바로 반영이 안되는 경우가 있습니다.
렌더링 반영 오류는 저도 겪었는데 어떤 문제였는지 기억이 안나네요.. 렌더링 과정을 좀 더 최적화 해보시면 좋을 것 같습니다.
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.
코드 리뷰 반영 완료
📌 과제 설명
바닐라JS를 이용한 노션 클로닝 프로젝트

기본적인 요구사항을 충족시키는 것에 집중하였습니다.
배포 사이트 : notion-cloning-vanilla-js.vercel.app
👩💻 요구 사항과 구현 내용
바닐라 JS만을 이용해 노션을 클로닝합니다.
기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
코드가 너무 구조화되어 있지 않고, 가독성이 안좋습니다.
많은 고민을 하였지만, 과제 제출 시간과 많은 로직을 한 번에 신경쓰기 힘들어 방대한 양의 코드를 잘 정리하지 못했습니다.
코드 구조화에 대해 피드백 주시면 감사하겠습니다!