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 과제 #52

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

Conversation

jkea1
Copy link

@jkea1 jkea1 commented Jul 6, 2023

📌 과제 설명

바닐라 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를 불러와 편집기에 로딩합니다.

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

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

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

@jkea1 jkea1 requested a review from off-programmers July 6, 2023 15:13
@jkea1 jkea1 self-assigned this Jul 6, 2023
@jkea1 jkea1 changed the title 4/#5 jkea1 working [Mission5/정진경] Project_Notion_VanillaJs 과제 Jul 6, 2023
Copy link

@off-programmers off-programmers left a comment

Choose a reason for hiding this comment

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

아직 덜 완성된것 같아서 작업이 완료되면 궁금한점 슬랙으로 직접 질문 남겨주세요! 화이팅입니다.

@@ -0,0 +1,9 @@
// .eslintrc.json

Choose a reason for hiding this comment

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

린트 규칙이 두개가 있는데 이유가 있나요?_?


<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">

Choose a reason for hiding this comment

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

meta 설정에 있어 어떤 값들이 들어갈 수 있는지 이참에 찾아보면 좋을것 같네요~

src/storage.js Outdated
}
};

// 넣을때는 JSON.stringify()로 직렬화해서 넣어야 한다.

Choose a reason for hiding this comment

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

요 파일에 있는 주석들은 제거되어도 좋을것 같아요~

src/router.js Outdated
Comment on lines 14 to 21
//url이 바꼈을때 감지하는 역할
export const push = (nextUrl) => {
console.log('확인', nextUrl);
window.dispatchEvent(
new CustomEvent(ROUTE_CHANGE_EVENT_NAME, {
detail: {
// detail에 url을 저장한다.
nextUrl,

Choose a reason for hiding this comment

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

요기 console이랑 주석도 제거되어도 괜찮을것 같아요!

Comment on lines +32 to +34
doc.documents &&
doc.documents
.map((childDoc) => {

Choose a reason for hiding this comment

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

nullish 병합 연산자 찾아보세요~

this.setState(updatedAllDocuments);
} else {
const { id } = clickedElement.dataset;
console.log(id);

Choose a reason for hiding this comment

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

요런 콘솔들은 제거해주세여~

Comment on lines +56 to +67
if (clickedElement.className === 'addBtn' && clickedElement) {
const { id } = clickedElement.dataset;
await createNewDocument(id);
const undatedAllDocuments = await getAllDocuments();
this.setState(undatedAllDocuments);
} else if (clickedElement.className === 'deleteBtn') {
const { id } = clickedElement.dataset;
await deleteDocument(id);
const updatedAllDocuments = await getAllDocuments();
this.setState(updatedAllDocuments);
} else if (clickedElement.className === 'createNewRootDocBtn') {
await createNewDocument();

Choose a reason for hiding this comment

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

클래스 이름으로 분기를 태우는건 추후 변경에 너무 취약합니다
명확한 타입을 지정해주세여

@jkea1 jkea1 force-pushed the 4/#5_jkea1_working branch from 50a52df to f675c82 Compare July 9, 2023 08:28
@doggopawer
Copy link

진경 님 죄송합니다. 저번에 커밋 고치신줄 알았는데 아닌건가요?
코드리뷰를 달려고 FilesChanged에 들어갔는데 제 맥북이 못버티는것 같습니다 ㅠㅠ
깃헙은 트리 구조의 리스트를 어떻게 렌더링하는지 모르겠지만 깊이우선 탐색을 사용한다고 하면
O(E+V)의 위력은 엄청난것 같네요 😂

@jkea1
Copy link
Author

jkea1 commented Jul 10, 2023 via email

Copy link

@MinwooP MinwooP left a comment

Choose a reason for hiding this comment

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

열심히 보고 리뷰를 써보긴 했으나 언제까지나 제 생각인 점 감안해주세요 ! 진경님 과제하시느라 엄청 고민하고 고생하신 거 제가 다 봤습니다 ㅠㅠ 정말 고생 많으셨어요 ㅠㅠㅠ 진경님도 느끼셨듯이 저도 이번 과제를 하면서 컴포넌트 구조와 각 컴포넌트에서 관리해줄 data들을 설계하고, 그 다음 구현에 들어가는 게 중요하다고 느꼈습니다 😢 나머지 기능들 구현하실 때에도 이런 부분 생각하시면서 하면 훨씬 수월할 것 같아요 !! 또 하시면서 헷갈리시거나 하는 거 있으시면 정말 언제든지 스터디룸 끌고 가셔도 됩니다 ㅎㅎㅎ 고생 너무 많으셨습니다 ~

};

export const updateDocumentTitle = (onUpdate) => {
window.addEventListener('title-update', () => {
Copy link

Choose a reason for hiding this comment

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

ROUTE_CHANGE_EVENT_NAME 이벤트 name은 상수화해주셨는데, 여기서 'title-update'는 상수화가 안 되어있네요 😢 정신 없으셔서 미처 확인 못하신 것 같습니다 !

};

this.route();
initRouter(() => this.route());
Copy link

Choose a reason for hiding this comment

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

url이 변경되야하는 상황일 때,

  1. custom event 발생시키기
  2. 이벤트 리스너에서 이를 감지
  3. pushState로 url 변경
  4. App의 route() 함수에서 바뀐 url에 따라 컴포넌트 렌더링
    => 현재 이런 순서로 진행되는 것 같은데

custom event를 따로 발생시키지 않아도, 특정 컴포넌트에서 pushState()후에 App의 route() 함수만 실행시켜주시면

  1. pushState()로 현재 url 변경
  2. App의 route() 함수에서 바뀐 url에 따라 컴포넌트 렌더링
    => 이렇게 2단계로도 충분할 것 같습니다 ! 😊

import DocumentEditComponent from './DocumentEditComponent.js';
import { updateDocumentTitle } from './router.js';

export default function DocumentsPage({ target }) {
Copy link

Choose a reason for hiding this comment

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

DocumentsPage와 ListComponenet 모두 각각 우측, 좌측에 표시되는 각 component를 의미하는 것 같은데, 접미사 -Page와 -Component를 통일해주시면 가독성이 훨씬 좋을 것 같아요 ! 😊

}
};

export const createNewDocument = async (id = null) => {
Copy link

Choose a reason for hiding this comment

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

유틸화 해놓으니 각 API 호출이 어떤 걸 의미하는 지 훨씬 명확한 것 같습니다 💡😗

Copy link

@doggopawer doggopawer left a comment

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 +27
<div data-id="${doc.id}">${doc.title} ${doc.id}</div>
<button data-id="${doc.id}" class="deleteBtn">delete</button>
<button data-id="${doc.id}" class="addBtn">add</button>

Choose a reason for hiding this comment

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

비구조화 할당을 사용해 보는게 어떨까요?

Comment on lines +55 to +67
const clickedElement = e.target;
if (clickedElement.className === 'addBtn' && clickedElement) {
const { id } = clickedElement.dataset;
await createNewDocument(id);
const undatedAllDocuments = await getAllDocuments();
this.setState(undatedAllDocuments);
} else if (clickedElement.className === 'deleteBtn') {
const { id } = clickedElement.dataset;
await deleteDocument(id);
const updatedAllDocuments = await getAllDocuments();
this.setState(updatedAllDocuments);
} else if (clickedElement.className === 'createNewRootDocBtn') {
await createNewDocument();

Choose a reason for hiding this comment

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

clickedElement가 있는지 먼저 확인해야하니까 차라리 clickedElement가 있는지 확인하는 조건절만 if로 가장 밖으로 빼는게 더 좋지 않을 까하는 생각입니다.

Comment on lines +17 to +18
editor.querySelector('[name=title]').value = this.state.title;
editor.querySelector('[name=content]').value = this.state.content;

Choose a reason for hiding this comment

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

setState 바깥에서 변수로 빼주는게 가독성에 조금 더 좋지 않을까 생각해봅니다

Copy link
Member

@sscoderati sscoderati left a comment

Choose a reason for hiding this comment

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

진경님 노션 클론 과제 고생하셨습니다! 종종 코드 업데이트 해주세요! 놀러올게요!

Copy link
Member

Choose a reason for hiding this comment

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

요거는 진경님의 개인 작업 환경과 관련된 설정 파일인 것 같아요!
이런 파일들은 보통 .gitignore에 등록하지 않을까 싶습니다..!

const clickedElement = e.target;
if (clickedElement.className === 'addBtn' && clickedElement) {
const { id } = clickedElement.dataset;
await createNewDocument(id);
Copy link
Member

Choose a reason for hiding this comment

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

createNewDocument는 async 함수여서 await을 사용하신 것 같은데, 결과값을 담아서 활용하고 계시지 않아서 await을 딱히 사용하지 않아도 될 것 같습니다!

this.setState(undatedAllDocuments);
} else if (clickedElement.className === 'deleteBtn') {
const { id } = clickedElement.dataset;
await deleteDocument(id);
Copy link
Member

Choose a reason for hiding this comment

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

위 createNewDocument와 비슷한 결의 함수인 것 같아서 원형을 살펴보고 왔는데요, 이 deleteDocument 함수 역시 따로 값을 반환하고 있지 않아서 await 키워드를 사용하지 않아도 될 것 같아요!

Comment on lines +29 to +32
const fetchDocsList = async () => {
const docs = await request('/');
};

Copy link
Member

Choose a reason for hiding this comment

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

docs를 받아서 어디에 활용하고 있을까요?

Comment on lines +33 to +36
this.render = async () => {
await fetchDocsList();
target.appendChild(page);
};
Copy link
Member

Choose a reason for hiding this comment

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

요 부분도 뭔가 이상합니다... 값을 반환하지 않는 fetchDocsList에 await 키워드가 적용되어있고, await을 사용하지 않는다면 굳이 async 함수일 필요가 없어보여요

Comment on lines +13 to +15
updateDocumentTitle(async () => {
await fetchDocsList();
});
Copy link
Member

Choose a reason for hiding this comment

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

여기도 마찬가지로 await으로 받은 결과를 반환하거나 활용하고 있지 않아요!

Comment on lines +22 to +25
this.render = async () => {
await fetchDocsList();
target.appendChild(page);
};
Copy link
Member

Choose a reason for hiding this comment

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

DocumentPage 컴포넌트에도 똑같은 함수가 있죠!

Comment on lines +45 to +59
export const deleteDocument = async (id) => {
await request(`/${id}`, {
method: 'DELETE',
});
};

export const putDocument = async (doc) => {
await request(`/${doc.id}`, {
method: 'PUT',
body: JSON.stringify({
title: doc.title,
content: doc.content,
}),
});
};
Copy link
Member

Choose a reason for hiding this comment

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

request의 반환값을 활용하지 않는다면 await을 쓰지 않아도 되지 않을까요?

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.

5 participants