Skip to content
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

Open
wants to merge 33 commits into
base: 4/#5_khakhid
Choose a base branch
from

Conversation

khakhiD
Copy link
Member

@khakhiD khakhiD commented Jul 6, 2023

🌎 배포 링크

https://vanilla-js-notion-clone-project.vercel.app/

📌 과제 설명

바닐라 JS만을 이용해 노션을 클로닝합니다.
기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.
글 단위를 Document라고 합니다. Document는 Document 여러개를 포함할 수 있습니다.

👩‍💻 요구 사항과 구현 내용

기본 요구사항

  • 화면 좌측에 Root Documents를 불러오는 API를 통해 루트 Documents를 렌더링합니다.
  • Root Document를 클릭하면 오른쪽 편집기 영역에 해당 Document의 Content를 렌더링합니다.
  • 해당 Root Document에 하위 Document가 있는 경우, 해당 Document 아래에 트리 형태로 렌더링 합니다.
  • Document Tree에서 각 Document 우측에는 + 버튼이 있습니다. 해당 버튼을 클릭하면, 클릭한 Document의 하위 Document로 새 Document를 생성하고 편집화면으로 넘깁니다.
  • 편집기에는 기본적으로 저장 버튼이 없습니다. Document Save API를 이용해 지속적으로 서버에 저장되도록 합니다.
  • History API를 이용해 SPA 형태로 만듭니다.
  • 루트 URL 접속 시엔 별다른 편집기 선택이 안 된 상태입니다.
  • /documents/{documentId} 로 접속시, 해당 Document 의 content를 불러와 편집기에 로딩합니다.

보너스 요구사항

  • 기본적으로 편집기는 textarea 기반으로 단순한 텍스트 편집기로 시작하되, 여력이 되면 div와 contentEditable을 조합해서 좀 더 Rich한 에디터를 만들어봅니다.
  • 편집기 최하단에는 현재 편집 중인 Document의 하위 Document 링크를 렌더링하도록 추가합니다.
  • 편집기 내에서 다른 Document name을 적은 경우, 자동으로 해당 Document의 편집 페이지로 이동하는 링크를 거는 기능을 추가합니다.

✅ PR 포인트 & 궁금한 점

  • 기본 요구사항에 충실히 준수하려고 했습니다.
  • 컴포넌트 구조를 깊게 할 수록 머리가 아파와서 최대한 단순히 하려고 했습니다.

✅ 리팩토링 반영 사항

  • 무의미한 eslint 설정 및 babel 프로젝트에서 삭제
    • jest 코드 작성을 못해서 eslint 관련 파일도 삭제했습니다. 다음 과제에서는 테스트 코드를 작성해보도록 하겠습니다.
  • 컴포넌트 내부에 사용되는 이벤트, 클래스 명 상수 처리
  • 방어 코드 추가
  • 조건문 표현 리팩토링 (옵셔널 체이닝 사용 등)
  • Editor 컴포넌트: querySelector를 한 번만 사용하기 위해서 요소 참조를 변수 방식으로 변경
  • deprecated 코드인 e.keyCode 대신 e.key로 리팩토링
  • debounce 함수 따로 구현 후 onEdit 함수에 적용
  • docsPage에서 document 클릭 시 두 번 네트워크 요청되는 부분 수정
  • 서버에 저장 중임을 알리는 로딩 아이콘 기능 추가
    • Loading 컴포넌트 추가
    • Editor 컴포넌트에서 keyup 이벤트 발생 시 Loading 렌더링
    • Loading 컴포넌트 관련 스타일 추가
  • 하위 컴포넌트들의 state 구조 수정
  • fontawesome 설치형 방식으로 변경 (index.html에서 cdn 관련 링크 및 스크립트 태그 삭제)
  • 패키지 설정 및 vite 설정 추가

Copy link
Member

@hyoribogo hyoribogo left a comment

Choose a reason for hiding this comment

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

동호님 잘 봤습니다.
파일 하나에 다 있으니 사실상 제일 보기 편했네요..... 😋

image

새로고침을 하면 이런 요청을 하던데 혹시 뭔지 아시나요 ?
그리고 이벤트 발생 했을 때 요청을 2번씩 보내더라고요 !!

코드에서 찾아보려 했지만 못 찾았네요 .. 죄송합니닷
2번씩 요청 보내지는 거 수정하면 좋을 거 같습니다 !

src/components/DocsEdit/Editor.js Outdated Show resolved Hide resolved
src/components/App.js Show resolved Hide resolved
src/components/App.js Show resolved Hide resolved
src/components/DocsEdit/EditorFooter.js Outdated Show resolved Hide resolved
src/utils/escapeHTML.js Outdated Show resolved Hide resolved
Copy link
Member

@dlwl98 dlwl98 left a comment

Choose a reason for hiding this comment

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

코드가 일관성있고 보기편했습니다. 강의에 굉장히 충실한 코드네요!
갈아엎기 전 코드도 궁금하네요

src/utils/escapeHTML.js Outdated Show resolved Hide resolved
Comment on lines 9 to 24
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)
}
Copy link
Member

Choose a reason for hiding this comment

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

디바운스 로직이 여기서만 쓰여서 분리할필요가 없지만 분리할 경우 테스트코드 작성 등 부수적인 득이 있습니다.
또한 의식적으로 도메인로직을 덜어내는 연습을 하는 것이 좋다고 생각해요. 저는 이런게 어렵더라구요.

Copy link
Member Author

Choose a reason for hiding this comment

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

아직 코드를 작성하면서 도메인 로직인 것과 아닌 것을 생각하면서 작성하는 습관이 부족한 것 같습니다.
리팩토링 기간에 의식해서 수정해보도록 하겠습니다. 감사합니다.

Copy link

Choose a reason for hiding this comment

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

디바운스 함수를 만들어서 사용해주면 좋을 것 같습니다:-)

Comment on lines 25 to 42
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()
}
Copy link
Member

Choose a reason for hiding this comment

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

setState라는 메서드명과는 관련없는 로직들이 들어있어요.
setState라는 이름을 그대로 둔다면 관련없는 로직들을 덜어내는것이 좋을 것 같고,
메서드명을 다르게 바꿔도 좋지않나 싶어요.
제 생각은 함수는 이름만으로 그 역할을 예상할 수 있어야 된다고 생각해요.
이름을 보고 예상한 것과 로직이 다르면 이해하기가 어려워지니까요.

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 코드를 작성하면서 헷갈리고 갇힌 부분이었는데, 짚어주셔서 감사합니다.

src/components/DocsEdit/Editor.js Outdated Show resolved Hide resolved
src/components/DocsEdit/Editor.js Outdated Show resolved Hide resolved
src/components/DocsEdit/Editor.js Outdated Show resolved Hide resolved
src/components/App.js Show resolved Hide resolved
src/components/DocsEdit/EditPage.js Outdated Show resolved Hide resolved
Copy link

@Jeong-Taeho Jeong-Taeho left a comment

Choose a reason for hiding this comment

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

동호님 과제하느라 수고하셨습니다! 깔끔하게 잘 정리되어 있어서 이해하기 편했던 것 같아요! 아는게 많이 없어서 리뷰를 제대로 못한 것 같네요😂 저도 효리님 리뷰보고 네트워크가 2번씩 계속 호출되는 이유를 찾아보려 했는데 어렵네요...

src/components/App.js Show resolved Hide resolved
src/components/DocsEdit/Editor.js Outdated Show resolved Hide resolved
Copy link
Member

@JIY00N2 JIY00N2 left a comment

Choose a reason for hiding this comment

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

동호님 코드가 강의와 유사해서 이해하기 편했습니다!
되게 디테일한 부분도 신경 쓰신 게 느껴졌습니다.
다른 팀원분들이 리뷰를 많이 해주셔서 리뷰가 적은 점 죄송합니다😥
노션 프로젝트 과제 하느라 고생 많으셨습니다!

index.html Outdated Show resolved Hide resolved
src/utils/router.js Outdated Show resolved Hide resolved
src/components/DocsEdit/Editor.js Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
}

$footer.addEventListener("click", (e) => {
const $title = e.target.closest(".sub-docs")
Copy link

Choose a reason for hiding this comment

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

sub-doc을 상수화 하고 지정된 컴포넌트와 여기에서 사용해도 좋을 것 같아용

Copy link
Member Author

Choose a reason for hiding this comment

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

[메모] class 명으로 요소를 불러올 때와 같은 경우에는 상수화해서 사용하기...
감사합니다 🥰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants