-
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 과제 #51
base: 4/#5_khakhid
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.
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/components/App.js
Outdated
const onEdit = async ({ id, title, content }) => { | ||
if (timer !== null) { | ||
clearTimeout(timer) | ||
} | ||
timer = setTimeout(async () => { | ||
await request(`/documents/${id}`, { | ||
method: "PUT", | ||
body: JSON.stringify({ | ||
title, | ||
content, | ||
}), | ||
}) | ||
await docsPage.render() | ||
// editPage.render() | ||
}, 1000) | ||
} |
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.
아직 코드를 작성하면서 도메인 로직인 것과 아닌 것을 생각하면서 작성하는 습관이 부족한 것 같습니다.
리팩토링 기간에 의식해서 수정해보도록 하겠습니다. 감사합니다.
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/components/DocsEdit/EditPage.js
Outdated
this.setState = async (nextState) => { | ||
if (this.state.id !== nextState.id) { | ||
this.state = nextState | ||
await fetchDocument() | ||
return | ||
} | ||
this.state = nextState | ||
|
||
editor.setState( | ||
this.state || { | ||
title: "", | ||
content: "", | ||
}, | ||
) | ||
|
||
editorFooter.setState(this.state.documents) | ||
this.render() | ||
} |
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.
setState라는 메서드명과는 관련없는 로직들이 들어있어요.
setState라는 이름을 그대로 둔다면 관련없는 로직들을 덜어내는것이 좋을 것 같고,
메서드명을 다르게 바꿔도 좋지않나 싶어요.
제 생각은 함수는 이름만으로 그 역할을 예상할 수 있어야 된다고 생각해요.
이름을 보고 예상한 것과 로직이 다르면 이해하기가 어려워지니까요.
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.
동호님 과제하느라 수고하셨습니다! 깔끔하게 잘 정리되어 있어서 이해하기 편했던 것 같아요! 아는게 많이 없어서 리뷰를 제대로 못한 것 같네요😂 저도 효리님 리뷰보고 네트워크가 2번씩 계속 호출되는 이유를 찾아보려 했는데 어렵네요...
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.
동호님 코드가 강의와 유사해서 이해하기 편했습니다!
되게 디테일한 부분도 신경 쓰신 게 느껴졌습니다.
다른 팀원분들이 리뷰를 많이 해주셔서 리뷰가 적은 점 죄송합니다😥
노션 프로젝트 과제 하느라 고생 많으셨습니다!
} | ||
|
||
$footer.addEventListener("click", (e) => { | ||
const $title = e.target.closest(".sub-docs") |
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.
sub-doc을 상수화 하고 지정된 컴포넌트와 여기에서 사용해도 좋을 것 같아용
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.
[메모] class 명으로 요소를 불러올 때와 같은 경우에는 상수화해서 사용하기...
감사합니다 🥰
- querySelector를 한 번만 사용하기 위해서 element 참조를 변수 방식으로 변경 - setState 함수의 value 수정 부분을 render 함수 내부로 위치 변경 - keyup 이벤트에서 e.keyCode를 e.key로 변경 - input 이벤트와 key 이름을 상수화
- (this.state.length > 0) => (this.state.length)
🌎 배포 링크
https://vanilla-js-notion-clone-project.vercel.app/
📌 과제 설명
바닐라 JS만을 이용해 노션을 클로닝합니다.
기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.
글 단위를 Document라고 합니다. Document는 Document 여러개를 포함할 수 있습니다.
👩💻 요구 사항과 구현 내용
기본 요구사항
보너스 요구사항
✅ PR 포인트 & 궁금한 점
✅ 리팩토링 반영 사항