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_VanilaJs 과제 #58

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

Conversation

Eosdia
Copy link

@Eosdia Eosdia commented Jul 7, 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를 불러와 편집기에 로딩합니다.

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

과제는 왼쪽의 sidebar 와 오른쪽의 editor 로 구성되어 있습니다.
루트 url (' / ') 에서는 sidebar 에는 document 리스트를 보여주고, editor에서는 새로운 문서를 생성할 수 있는 편집기를 보여줍니다.
sidebar에서 문서를 조회할 수 있고, 문서 안의 하위 문서를 생성할 수 있고, 삭제 할 수 있습니다.
새로운페이지를 클릭하면 하위문서가 아닌 루트 문서를 생성 할 수 있습니다.
editor에서는 새로운 문서를 작성 할 수 있고, 기존의 문서를 편집 할 수 있습니다.

default.mov

✅ PR 포인트 & 궁금한 점

바닐라 js로 구현하는게 미숙하여 과제를 진행하는데 시간이 많이 걸려서 진행 일정을 맞추지 못한점 죄송합니다.
간단한 동작을 목표로 일단 문서조회, 생성, 삭제 기능과 편집 기능만 구현하였습니다.
주석, 커밋 메세지를 신경쓰지 못한점. 죄송합니다...

  • SPA를 위한 history api를 보완할 예정입니다..

과제를 진행하다가 어떻게 구현하는지에 대해서 생각을 하는데 시간이 많이 걸리는 것 같습니다.

  • 구현에 대한 고민하는 시간은 얼마나 걸리는게 좋을까요? 또 진행하다가 비효율적으로 설계된것 같을때는 다시 엎고 했는데 더 나은 방법이 있을까요?

Copy link

@JeongWuk JeongWuk 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 +44
onSelectDocument: (documentId) => {
const nextState = { documentId };
console.log(nextState);
editPage.setState(nextState);
},
onCreateDocument: (documentId) => {
const parent = documentId;
const nextState = {
documentId: 'new',
parent,
};
editPage.setState(nextState);
},
onRemoveDocument: async (documentId) => {
console.log('remove');
await request(`/documents/${documentId}`, {
method: 'DELETE',
});
documentListPage.setState();
},

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.

👍👍👍👍👍 함수명 덕분에 코드를 이해하기 좋았습니당!!

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 +19 to +48
this.render = () => {
const $ul = document.createElement('ul');

this.state.map((documentInfo) => {
const $li = document.createElement('li');
$li.className = 'document_li';
$li.setAttribute('data-id', documentInfo.id);

const $title = document.createElement('span');
$title.textContent = documentInfo.title;
$title.className = 'title';
$li.appendChild($title);

const $createBtn = document.createElement('button');
$createBtn.textContent = '+';
$createBtn.className = 'createBtn';
$li.appendChild($createBtn);

const $removeBtn = document.createElement('button');
$removeBtn.textContent = '-';
$removeBtn.className = 'removeBtn';
$li.appendChild($removeBtn);

if (documentInfo.documents.length > 0) {
new DocumentList({
$target: $li,
initialState: documentInfo.documents,
});
}
$ul.appendChild($li);

Choose a reason for hiding this comment

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

innerHTML 대신에 appendChild 너무 잘 쓰시는 것 같아요... 저는 피드백 받아도 아직 익숙하지 않네요ㅠㅠ 나중에 기회가 된다면 배워보고 싶네요!

Copy link
Member

@HongGunWoo HongGunWoo left a comment

Choose a reason for hiding this comment

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

코드 잘 봤습니다. 변수명이 직관적이여서 금방 이해할 수 있었어요! 저도 아직이긴 한데 localStorage 이용해서 서버 업로드에 실패하더라도 작성한 내용이 local에 저장될 수 있게 하면 더 좋을 것 같아요!!

Comment on lines +11 to +24
new ListHeader({
$target: $sidebar,
onNewDocument: () => {
console.log('새로운 문서');
const nextState = {
documentId: 'new',
parent: null,
};
editPage.setState(nextState);
},
});

const documentListPage = new DocumentListPage({
$target: $sidebar,
Copy link
Member

Choose a reason for hiding this comment

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

p3;
SideBar에 해당되는 컴포넌트들은 SideBar 폴더의 DocumentListPage에서 컴포넌트를 생성하는 방향도 좋을 것 같아요!

Comment on lines +25 to +44
onSelectDocument: (documentId) => {
const nextState = { documentId };
console.log(nextState);
editPage.setState(nextState);
},
onCreateDocument: (documentId) => {
const parent = documentId;
const nextState = {
documentId: 'new',
parent,
};
editPage.setState(nextState);
},
onRemoveDocument: async (documentId) => {
console.log('remove');
await request(`/documents/${documentId}`, {
method: 'DELETE',
});
documentListPage.setState();
},
Copy link
Member

Choose a reason for hiding this comment

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

👍👍👍👍👍 함수명 덕분에 코드를 이해하기 좋았습니당!!

Comment on lines +12 to +14
if (Array.isArray(initialState)) {
this.state = initialState;
}
Copy link
Member

Choose a reason for hiding this comment

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

initialState validation 과정 잊었는데 저도 추가해야겠네요! 👍👍👍

Comment on lines +52 to +70
$ul.addEventListener('click', (e) => {
const $li = e.target.closest('li');
if ($li !== null) {
const documentId = $li.dataset.id;

// document 선택
if (e.target.className === 'title') {
onSelectDocument(documentId);
}
//하위 document 생성
if (e.target.className === 'createBtn') {
onCreateDocument(documentId);
}
// document 삭제
else if (e.target.className === 'removeBtn') {
onRemoveDocument(documentId);
}
}
});
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

@khj0426 khj0426 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 +44
onSelectDocument: (documentId) => {
const nextState = { documentId };
console.log(nextState);
editPage.setState(nextState);
},
onCreateDocument: (documentId) => {
const parent = documentId;
const nextState = {
documentId: 'new',
parent,
};
editPage.setState(nextState);
},
onRemoveDocument: async (documentId) => {
console.log('remove');
await request(`/documents/${documentId}`, {
method: 'DELETE',
});
documentListPage.setState();
},
Copy link

Choose a reason for hiding this comment

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

👍 👍 👍 👍 👍 👍 👍 👍

return;
}
this.state = nextState;
editor.setState(this.state.document || { title: '', content: '' });
Copy link

Choose a reason for hiding this comment

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

OR연산자 사용하신 거 좋습니당!!! 👍

$target.appendChild($header);
this.render = () => {
const $userInfo = document.createElement('h3');
$userInfo.textContent = 'Notion 과제중...';
Copy link

Choose a reason for hiding this comment

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

지금도 괜찮지만, 이런 문자열도 상수화를 해주면 더더 좋을 거 같아요!..!!

Copy link

@feel5ny feel5ny left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 💯
전반적으로 구조가 깔끔하고 보기가 좋았습니다 👍

구현에 대한 고민하는 시간은 얼마나 걸리는게 좋을까요?

저번에 테크스펙에 대해서 말씀드렸다싶이, 제일 좋은 것은 미리 구조에 대해서 "계획"하면 가장 베스트입니다. 코드를 작성하며 계획하냐, 문서에서 수도코드로 작성하며 미리 계획하냐의 순서차이인데, 리소스가 덜 드는 방향은 수도코드일 것 같긴하네요 ㅎㅎ 그렇다고 계획에서 백프로 구조화는 힘들기때문에 어느정도 밑그림을 그리고 개발을 진행하는 것을 추천드립니다.

또 진행하다가 비효율적으로 설계된것 같을때는 다시 엎고 했는데 더 나은 방법이 있을까요?

더 나은 방법이라하시면, "다시 엎는 행위"를 줄이고 싶으신거겠지요? 첫 질문의 대답으로 갈음할 수 있을 것 같습니다. 더불어서 "사전 계획"을 다른 분이 리뷰를 해준다면 더더욱 사전에 검증이 탄탄해 질 것 같습니다.

padding: 8px;
border-bottom: 1px solid black;
}
.sidebar_newDocument {
Copy link

Choose a reason for hiding this comment

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

p3;
네이밍 컨벤션을 하나로 통일하면 좋을 듯 합니다.
현재는 snake케이스와 camel케이스가 혼용되어있네요!

new ListHeader({
$target: $sidebar,
onNewDocument: () => {
console.log('새로운 문서');
Copy link

Choose a reason for hiding this comment

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

p3;
PR업로드시에는 콘솔 제거해주세요~

const $userInfo = document.createElement('h3');
$userInfo.textContent = 'Notion 과제중...';

const $newDocument = document.createElement('div');
Copy link

Choose a reason for hiding this comment

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

UI상 button태그가 더 어울려보이네요 🙏
image

Comment on lines +55 to +64
if (this.state.documentId !== nextState.documentId) {
this.state = nextState;
if (this.state.documentId === 'new') {
this.render();
editor.setState({ title: '', content: '' });
} else {
await fetchDocument();
}
return;
}
Copy link

Choose a reason for hiding this comment

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

if가 이중중첩이네요 🤔
nested 구조가 가독성을 낮출 수 있는데, 리팩토링해보면 어떨까요~?

clearTimeout(timer);
}
timer = setTimeout(async () => {
const isNew = this.state.documentId === 'new';
Copy link

Choose a reason for hiding this comment

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

변수명 좋습니다 👍


$editor.addEventListener('keyup', (e) => {
const name = e.target.getAttribute('name');
if (this.state[name] !== undefined) {
Copy link

Choose a reason for hiding this comment

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

p3;

Suggested change
if (this.state[name] !== undefined) {
if (this.state[name]) {

truthy로 처리 가능할 것 같네요

const name = e.target.getAttribute('name');
if (this.state[name] !== undefined) {
const nextState = {
...this.state,
Copy link

Choose a reason for hiding this comment

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

👍

};

$editor.addEventListener('keyup', (e) => {
const name = e.target.getAttribute('name');
Copy link

Choose a reason for hiding this comment

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

이벤트 위임을 잘해주신 것 같아 좋습니다 👍
변수명이 조금더 직관적이면 좋을 것 같네요!

this.route = () => {
$target.append($sidebar, $editor);
const { pathname } = window.location;
if (pathname === '/') {
Copy link

Choose a reason for hiding this comment

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

p4;
isNew로 처리하셨던 것처럼 변수명으로 표현하면 읽기 편할 것 같습니다!

Comment on lines +53 to +70
const $li = e.target.closest('li');
if ($li !== null) {
const documentId = $li.dataset.id;

// document 선택
if (e.target.className === 'title') {
onSelectDocument(documentId);
}
//하위 document 생성
if (e.target.className === 'createBtn') {
onCreateDocument(documentId);
}
// document 삭제
else if (e.target.className === 'removeBtn') {
onRemoveDocument(documentId);
}
}
});
Copy link

Choose a reason for hiding this comment

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

👍
early return으로 작성되면 좀더 깔끔할 것 같군요!

Suggested change
const $li = e.target.closest('li');
if ($li !== null) {
const documentId = $li.dataset.id;
// document 선택
if (e.target.className === 'title') {
onSelectDocument(documentId);
}
//하위 document 생성
if (e.target.className === 'createBtn') {
onCreateDocument(documentId);
}
// document 삭제
else if (e.target.className === 'removeBtn') {
onRemoveDocument(documentId);
}
}
});
const $li = e.target.closest('li');
if (!$li) return;
const documentId = $li.dataset.id;
// document 선택
if (e.target.className === 'title') {
onSelectDocument(documentId);
}
//하위 document 생성
if (e.target.className === 'createBtn') {
onCreateDocument(documentId);
}
// document 삭제
if (e.target.className === 'removeBtn') {
onRemoveDocument(documentId);
}
});

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