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

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

Conversation

eeseung
Copy link

@eeseung eeseung commented Jul 6, 2023

📌 과제 설명

노션 클로닝 프로젝트

바로 가기

Project_Notion_VanillaJS.mov

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

기본 요구사항

바닐라 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 포인트 & 궁금한 점

  • 문서 수정 시 수정된 제목을 DocumentList에도 반영하다 보니 NotionPage에서 모든 컴포넌트를 관리하게 되었는데 괜찮은 방법인가요? 더 나은 구조가 있는지 궁금합니다.
  • 불필요한 API 호출이 있는지, 렌더링 관점에서 개선이 필요한 부분이 있는지 궁금합니다.
  • 코드에서 안 좋은 습관이나 보완하면 좋을 부분을 피드백 받고 싶습니다!

@eeseung eeseung changed the title [Mission5/이승희] Project_Notion_VanillaJs 과제 [Mission5/이승희] Project_Notion_VanillaJS 과제 Jul 6, 2023
@eeseung eeseung requested a review from pers0n4 July 6, 2023 17:13
Copy link

@from1to2 from1to2 left a comment

Choose a reason for hiding this comment

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

코드 작성하느라 수고하셨습니다! 다음 과제 화이팅입니다! 👍

Choose a reason for hiding this comment

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

assets 폴더에 각종 icon 모아두면 좋겠네요 👍

export const INIT_DOCUMENT = {
title: '',
content: '',
};

Choose a reason for hiding this comment

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

반복되는 부분을 줄이기 위해서, 의미를 명시적으로 나타내기 위해서 상수로 따로 모아두면 정말 좋은거 같네요 👍

Copy link
Member

@HeeSeok-kim HeeSeok-kim left a comment

Choose a reason for hiding this comment

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

배포 페이지 들어갔을 때 노션인 줄 알았습니다 👍

배포 페이지에서 새로고침이 404: NOT_FOUND 페이지가 표시됩니다.

Copy link
Member

@pers0n4 pers0n4 left a comment

Choose a reason for hiding this comment

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

문서 수정 시 수정된 제목을 DocumentList에도 반영하다 보니 NotionPage에서 모든 컴포넌트를 관리하게 되었는데 괜찮은 방법인가요? 더 나은 구조가 있는지 궁금합니다.

레이아웃을 어떤 구조로 가져갈 것이냐에 따라서 달라질 수 있는 부분이긴 한데, 일단 Page에서 모든 컴포넌트를 관리하는 방법도 유효한 접근법이긴 합니다.
다만 현재 Page 컴포넌트가 너무 비대한 지분을 차지하고 있기도 하고, 컴포넌트를 바라보는 관점에 따라 레이아웃의 계층을 바꿀만한 여지는 있어 보입니다.
가령 현재 라우팅을 진행할 때 CustomEvent를 활용하는 것처럼, 헤더의 타이틀이 바뀔 수 있는 케이스에 대해 이벤트를 정의해두고 Header에서 해당 이벤트가 감지되었을 때 내용을 업데이트하는 식으로 디커플링을 수행할 수도 있습니다.

Comment on lines +5 to +11
const res = await fetch(`${API_END_POINT}${url}`, {
...options,
headers: {
'Content-Type': 'application/json',
'x-username': USER_NAME,
},
});
Copy link
Member

Choose a reason for hiding this comment

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

만약 options에 header가 포함된다면 어떻게 될까요? 👀

Comment on lines +17 to +32
this.route = () => {
const { pathname } = window.location;

if (pathname === '/') {
notionPage.setState({
...notionPage.state,
documentId: null,
});
} else if (pathname.indexOf('/documents/') === 0) {
const [, , id] = pathname.split('/');
notionPage.setState({
...notionPage.state,
documentId: parseInt(id),
});
}
};
Copy link
Member

Choose a reason for hiding this comment

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

동일한 프로토타입을 가진 인스턴스에서 모두 활용할 수 있는 메서드는 프로토타입 객체에 선언하는 것이 좋습니다. this 속성으로 초기화한다면 인스턴스가 생성될 때마다 매번 초기화가 이루어집니다.

$editor.classList.add('editor');
$target.appendChild($editor);

let isInitialize = false;
Copy link
Member

Choose a reason for hiding this comment

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

initialize의 경우 동사이기 때문에 변수명으로 잘 쓰이지는 않고, is와 should 등의 접두어로 시작되는 상태를 나타내는 변수의 경우 축약어를 사용하는 isInit, 시제가 드러나는 isInitialized, isInitializing 등의 네이밍이 주로 쓰이곤 합니다. 초기화뿐만 아니라 상태를 표현하는 다른 플래그 변수도 비슷합니다.

Comment on lines +31 to +44
$editor.addEventListener('keyup', (e) => {
const { target } = e;
const name = target.getAttribute('name');

if (this.state[name] !== undefined) {
const nextState = {
...this.state,
[name]: target.value,
};

this.setState(nextState);
onEditing(this.state);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

불필요하다고 말할 필요까지는 없을 것 같지만 🤔, throttling 기법을 적용한다면 효율을 높일 수 있을 것 같네요.

Comment on lines +15 to +50
const renderDocuments = (documents, depth) => {
return `
${
documents.length > 0
? documents
.map((document) => {
const isExpanded = this.state.openedDocuments.indexOf(document.id) > -1;
return `
<li data-id="${document.id}" class="document-list-item">
<div class="document-item ${document.id === this.state.documentId ? 'selected' : ''}"
style="padding:2px 10px 2px ${depth * 14 + 5}px;">
<button class="toggle-button ${isExpanded ? 'expanded' : ''}"></button>
<div class="document-title">
${document.title.length === 0 ? '제목 없음' : document.title}
</div>
<div class="document-button-wrapper">
<div>
<button class="delete-button"></button>
<button class="add-button"></button>
</div>
</div>
</div>
<ul class="${isExpanded ? 'expanded' : ''}">
${renderDocuments(document.documents, depth + 1)}
</ul>
</li>`;
})
.join('')
: `
<li class="document-list-none" style="padding:2px 10px 2px ${depth * 14 + 15}px;">
하위 페이지 없음
</li>
`
}
`;
};
Copy link
Member

Choose a reason for hiding this comment

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

html 때문에 함수 라인이 길어지는데 map을 통해 실행되는 함수를 한 번 더 분리해주면 조금 더 깔끔하게 유지할 수 있지 않을까요?

Comment on lines +194 to +226
const fetchDocuments = async () => {
const documents = await request('/documents');

this.setState({
...this.state,
documents,
});
};

const fetchDocument = async () => {
const { documentId } = this.state;

if (documentId) {
const document = await request(`/documents/${documentId}`);

const tempDocument = getItem(documentLocalSaveKey, INIT_DOCUMENT);

if (tempDocument.tempSaveDate && tempDocument.tempSaveDate > document.updatedAt) {
if (confirm('저장되지 않은 임시 데이터가 있습니다. 불러올까요?')) {
this.setState({
...this.state,
document: tempDocument,
});
return;
}
}

this.setState({
...this.state,
document,
});
}
};
Copy link
Member

Choose a reason for hiding this comment

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

api를 호출하는 부분을 별도의 모듈(흔히 service라고 부름)로 분리하면 NotionPage의 라인 수도 줄이고 api를 호출하는 코드의 재사용성을 높일 수 있을 것 같습니다.

Comment on lines +229 to +234
$target.appendChild($documentContainer);

// 루트 url인 경우 노션 컨테이너 렌더링 X
if (this.state.documentId === null) {
$target.removeChild($documentContainer);
}
Copy link
Member

Choose a reason for hiding this comment

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

append했다가 remove하는 것보다는 조건에 맞을 때만 append하는 방식은 안 되는 걸까요? 🤔

Comment on lines +158 to +169
this.setState = async (nextState) => {
if (this.state.documentId !== nextState.documentId) {
documentLocalSaveKey = `${TEMP_DOCUMENT_KEY(nextState.documentId)}`;
this.state = nextState;

await fetchDocument();
this.render();

return;
}

this.state = nextState;
Copy link
Member

Choose a reason for hiding this comment

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

크게 중요한 부분은 아니지만 어느 분기에서나 항상 수행되는 this.state = nextState 구문은 아예 if문 위로 올리면 불필요한 코드를 줄이고, if문 내의 역할이 더욱 명확해질 것으로 보입니다.

Comment on lines +83 to +95
if (id === this.state.documentId) {
if (this.state.documents.length > 0) {
push(`/documents/${this.state.documents[0].id}`);
} else {
push('/');
}
} else {
if (this.state.documentId) {
push(`/documents/${this.state.documentId}`);
} else {
push('/');
}
}
Copy link
Member

Choose a reason for hiding this comment

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

조건문의 분기 구조는 가능하면 최대한 flat하게 만들어주는 게 좋습니다.
더 나아가서는 id === this.state.documentId && this.state.documents.length > 0처럼 복잡한 조건 판단식의 경우는 함수로 추출해주면 분기 구조를 더욱 명확하게 파악할 수 있습니다.

Suggested change
if (id === this.state.documentId) {
if (this.state.documents.length > 0) {
push(`/documents/${this.state.documents[0].id}`);
} else {
push('/');
}
} else {
if (this.state.documentId) {
push(`/documents/${this.state.documentId}`);
} else {
push('/');
}
}
if (id === this.state.documentId && this.state.documents.length > 0) {
push(`/documents/${this.state.documents[0].id}`);
} else if (this.state.documentId) {
push(`/documents/${this.state.documentId}`);
} else {
push('/');
}

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.

4 participants