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

[Mission4/한수화] - Project_Notion_VanillaJS #22

Open
wants to merge 27 commits into
base: 3/#4_suhwa
Choose a base branch
from

Conversation

live-small
Copy link
Member

@live-small live-small commented Nov 16, 2022

📌 과제 설명

자바스크립트로 노션 만들기

코드 실행방법

  1. src 폴더 안에 파일을 추가해주세요 (파일은 오프팀 슬랙 채널에 올려두었습니다 - 링크연결)
  2. 터미널에 아래 명령어를 넣어주세요.
npx serve -s 

실행영상

2022-11-19.7.11.57.mov

구조

notion

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

라우팅

  • / 접속 시, 왼쪽엔 document 트리, 오른쪽엔 "환영문구"를 보여준다.
  • /documents/{documentId} 접근 시, 왼쪽엔 document 트리, 오른쪽엔 해당 documetId의 content를 보여준다.
  • 유효하지 않은 documentId, 그 외의 URL은 모두 404페이지를 보여준다.

(왼쪽에 위치한)document 트리

  • document를 클릭하면, 오른쪽에 document의 content를 볼 수 있다.
  • 루트 document 좌측에 위치한 화살표를 클릭하면, 하위 document를 볼 수 있다.
  • 하위 document가 없을 경우, 하위 문서가 없습니다를 보여준다.
  • document 우측에 위치한 +버튼을 클릭하면, 클릭한 document 하위에 새 document를 생성하고, 오른쪽에 새로운 문서 페이지를 보여준다.
  • document 우측에 위치한 X버튼을 클릭하면, 해당 document가 삭제된다.
    • 추가구현 하위 document가 있다면, 하위 document까지 다 삭제된다.
    • 현재 보고있는 페이지가 삭제될 경우, 첫번째 문서 페이지로 이동한다.
    • 현재 삭제한 페이지가 유일한 페이지일 경우, / 로 이동한다.
  • 트리 맨 아래, 페이지 추가하기를 누르면, root에 페이지가 추가된다.

(오른쪽 위치) document Content

  • 해당 document의 content 편집할 수 있는 편집기가 있다.
  • 편집기는 지속적으로 서버에 저장된다 (Document Save API 이용해 자동저장 구현)

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

  1. 로컬 스토리지를 활용할만한 포인트가 있을까요?
  2. 불필요한 렌더링이 발생하는 지점이 있을까요?
  3. 아래 코드와 함께 코멘트로 남겨뒀습니다!

루트 document의 우측 화살표를 클릭하면, 하위 document를 볼 수 있도록 details 태그 이용
열렸던 문서 목록은 새로고침해도 유지된다 (로컬 스토리지에서 열린 문서 목록 상태값 저장함
editor page의 title 수정 시, 지속적으로 반영되도록 연결함
isOpen state 상태값을 붙이는 함수, 재귀적으로 돌도록 수정
마지막 이벤트가 실행되도록 지연시킬 때, 문서 컴포넌트 내 상태값은 지연시키지말고 계속 반영하도록 함
documents: [],
};

export const DOCUMENT_ISOEPN_LOCAL_KEY = 'isOpen';

Choose a reason for hiding this comment

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

오타용

Comment on lines +4 to +8
/**
*
* @param targetDocument - [{id, title, documents},{...}]
* @returns result - [id, id...]
*/

Choose a reason for hiding this comment

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

뭔가 jsdoc을 사용해보려는 연습이였던걸까요 ㅎㅎ

setItem(DOCUMENT_ISOEPN_LOCAL_KEY, nextIsOpenState);
};

export const $ = (selector) => {

Choose a reason for hiding this comment

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

음 이렇게 만들어서 사용하는것도 나쁘지는 않지만...
혹시 수화님의 의도가 아니게 jquery가 포함되어있을 경우 네임스페이스가 겹치게 되서 뭔가 의도와 다르게 될 수도 있습니다 ㅎㅎ
처음 코드를 접하는 분들은 jQuery를 사용하는 구나 라고 착각할수도 있구요!
오해의 여지는 줄일수 있다면 줄이는게 좋아보입니다.

@@ -0,0 +1,45 @@
import { DOCUMENT_ISOEPN_LOCAL_KEY } from './constants.js';

Choose a reason for hiding this comment

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

유틸이라는 폴더 하위에 있는 파일이라면 index.js로 만들어버리거나 명확하게 어떤 역할인지 폴더 이름을 정하면 좋을것 같아요

@@ -0,0 +1,27 @@
import { apiKey, API_END_POINT } from '../apiKey.js';

Choose a reason for hiding this comment

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

api 코드들은 유틸에 들어갈만한 성격의 함수들은 아닌거 같아요! 특히 updateDocumentContent가 그런것 같은데요. 해당 함수의 위치를 서비스같은 폴더를 만들어 관리하거나 하면 좋을것 같아요.

코드 내에서 request도 직접 사용하고 있다보니 같이 옮겨서 사용해도 좋을것 같네요. 아예 코드단에서는 url 조차 넘기지 말고 서비스 파일 내에서 작성하도록 하는것도 좋아보여요.

request(/${id}); 를 예로 들면 그냥 getDocumentById같은 메서드를 만들어서 컴포넌트 레벨에서 사용하고 request나 url은 서비스 파일에서 쥐고 관리하는거져. 그럼 훨씬 코드가 직관적으로 보이고 url관리도 좋을것 같습니다.


const currentIsOpenState = getItem(DOCUMENT_ISOEPN_LOCAL_KEY, []);
if (node.className === 'hide') {
// 닫혔으면 -> 로컬에서 빼기

Choose a reason for hiding this comment

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

불필요한 주석은 제거하면 좋을것 같습니다 ㅎㅎ

case 'toggle':
$document.childNodes.forEach((node) => {
if (node.nodeName === 'UL') {
node.classList.toggle('hide');

Choose a reason for hiding this comment

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

string으로 비교를 계속한다면 상수화를 해서 쓰는게 훨씬 안전해보여요

this.render();
};

this.render = () => {

Choose a reason for hiding this comment

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

render같은 함수들이 외부에서 직접 호출될 일이 없다면 this바인딩을 해서 공개하지 않아도 괜찮을것 같아요

this.render();

$nav.addEventListener('click', (e) => {
if (e.target.id === 'root-add') {

Choose a reason for hiding this comment

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

요런 id들도 다 상수화해서 사용하면 좋을것 같네요 ㅎㅎ

Comment on lines +64 to +65
history.replaceState(null, null, nextRoute);
route();

Choose a reason for hiding this comment

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

전반적으로 이런 코드 호출들은 좀 더 추상화해서 사용하면 좋을것 같아요.
route에 대한 코드도 분리해서 관리되면 좋을것 같구요!

Comment on lines +45 to +56
if (confirm(`하위문서를 포함해 ${removeIdList.length}개가 삭제됩니다`)) {
await Promise.all(
removeIdList.map((id) =>
request(`/${id}`, {
method: 'DELETE',
})
)
);
deleteIsOpenState(removeIdList);
await fetchDocumentList();

if (removeIdList.includes(this.state.currentDocumentId)) {

Choose a reason for hiding this comment

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

if안에 함수가 점점 더 깊어지고 복잡해지고 있어서 early return으로 처리할 수 있는 함수들은 처리하고 함수로 분리해서 사용할 수 있도록 구성하면 좋을것 같아요

const editor = new Editor({
$container: $editor,
initialState: documentContentDefaultValue,
onEdit: debounce(updateDocument, 800),

Choose a reason for hiding this comment

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

요런 800 같은 값도 매직 넘버기 때문에 상수화 시켜서 사용해주세요

@@ -0,0 +1,65 @@
import { request, updateDocumentContent } from '../utils/api.js';

Choose a reason for hiding this comment

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

크게 중요하지 않을수 있지만 component와 page를 구분해서 좀 더 큰 역할을 담고 있다고 폴더로 나눠주면 좋을것 같아요

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.

리뷰 완료입니다. 고생하셨어요

Copy link
Member Author

@live-small live-small 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 +42 to +56
onDelete: async (id) => {
const targetDocument = await request(`/${id}`);
const removeIdList = getRemoveIdList([targetDocument]);
if (confirm(`하위문서를 포함해 ${removeIdList.length}개가 삭제됩니다`)) {
await Promise.all(
removeIdList.map((id) =>
request(`/${id}`, {
method: 'DELETE',
})
)
);
deleteIsOpenState(removeIdList);
await fetchDocumentList();

if (removeIdList.includes(this.state.currentDocumentId)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Document 삭제 시, 하위문서가 있는 경우를 처리하다 든 고민

  • 상황

    • document 삭제 버튼을 눌렀을 때, 해당 document를 삭제하는 로직입니다.
    • 주어진 API는 하위문서가 있는 문서를 삭제할 때, 하위문서가 모두 루트(최상위)로 이동합니다.
    • 저는 하위문서가 있는 경우에 하위문서까지 다 삭제하는 게 이용할 때 편할 거 같아 그렇게 구현했습니다.
  • 고민

    • 삭제할 문서 id를 이용해 하위 문서까지 삭제하려니.. 제가 생각한 방법이 2개 였는데요
    1. documentList에서 삭제할 문서 id를 탐색해, 삭제할 문서의 정보 획득 -> 삭제 진행
    2. 문서 id로 해당 문서의 정보 받아오는 api 호출(/document/{documentId})해, 문서정보 획득 -> 삭제 진행
    • 저는 2번 방법으로 진행했는데, 불필요한 api 호출로 보일 수도 있을 거 같더라구요.
    • 2번으로 진행한 이유는, 루트에서 깊은 하위 노드일 경우에 탐색 시간이 길어질 거 같아서 api 호출로 처리했습니다.
  • 추가로..

    • 지금은 getRemoveList로 삭제할 documentId를 배열로 받아와서
    • Promise.all로 삭제 api를 호출하고
    • deleteIsOpenState로 isOpen(열린 폴더 정보)상태값을 정리하다보니
    • 삭제할 리스트를 2번 돌기때문에, 한번에 처리하는 게 더 낫겠죠?

질문

다른 분들은 문서삭제 시, 하위 문서가 있는 경우에 어떻게 처리하셨나요?

Choose a reason for hiding this comment

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

상위로 올라가도록 하신분들이 더 많은것 같은데 직접 코드리뷰 하면서 보시면 좋을것 같습니다 ㅎㅎ
현재 문서에 child id 배열을 갖고 있고 재귀적으로 삭제를 실행하면 모두 대응가능할 것 같습니다.

Choose a reason for hiding this comment

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

한번에 삭제라는 추가 기능도 구현해주셨네요👍👍 개인적인 의견으로 사용자 입장에서 하위 문서는 유지하고 싶은 경우가 있을 거 같아서 사용자에게 모두 삭제 또는 해당 문서만 삭제 이런식으로 선택 사항을 주어도 좋을듯 싶어요 :)
그리고 저도 오프 멘토님과 동일하게 현재 선택한 문서의 하위 문서들의 배열을 저장해두었다가 삭제 처리 시 가져와 사용할 거 같습니다.

Comment on lines +78 to +81
onError: () => {
history.replaceState(null, null, '/404');
route();
},
Copy link
Member Author

Choose a reason for hiding this comment

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

URL(document/${documetId})로 접근 시, 잘못된 documentId일 경우

  • 저는 위와 같은 URL 접근 시, documentEditorPage에 documentId를 넘겨주고
  • documentEditorPage의 setState에서 documentId로 해당 문서 정보를 얻어오는데
  • 유효한 문서정보가 아닐 경우, 잘못된 documentId라고 판단하고, URL을 404로 넘깁니다.
  • 근데, 라우터를 최상위에서 관리하는 게 좋을 거 같아서, onError라는 함수를 props로 넘기게 되었습니다.

질문

라우터를 어떻게 관리하는 게 좋을까요? 저는 최상위(NotionApp)에서 관리하는 것으로 했는데, 기능이 많아지면 props로 넘기는 게 많을 거 같아서.. 강의에서 나온 것 처럼 customEvent로 한 곳에서 처리하는 것? 까지 생각해봤습니다.

Choose a reason for hiding this comment

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

네네 이런 케이스의 라우팅은 특별한 케이스를 제외하고는 한곳에서 관리하는게 좋아보입니다.

Comment on lines +23 to +48
this.setState = async (nextDocumentId) => {
if (nextDocumentId === null) {
this.currentDocumentId = nextDocumentId;
emptyContent.setIsVisible(true);
editor.setState({
...editor.state,
isVisible: false,
});
return;
}
if (nextDocumentId === this.currentDocumentId) {
return;
}

const documentContent = await request(`/${nextDocumentId}`);
if (!documentContent) {
onError();
return;
}
this.currentDocumentId = nextDocumentId;
emptyContent.setIsVisible(false);
editor.setState({
...documentContent,
isVisible: true,
});
};
Copy link
Member Author

Choose a reason for hiding this comment

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

같은 위치에 다른 UI를 보여주려면 ?

  • 상황
    • 왼쪽은 문서목록, 오른쪽은 선택된 문서 편집기Editor를 보여주는데..
    • 루트인 경우에선 오른쪽에 다른 UI(Editoror 환영문구)를 보여줘야겠다는 생각을 했습니다.
  • 고민
    • 같은 위치에, 다른 UI를 보여주려면 둘 다 innerHTML을 쓰는 경우가 있는데
    • 둘 다 innerHTML을 쓰면, Editor에 문서를 수정할 때도 Editor이 다시 그려지기때문에..글을 쓰다가 cursor가 끊기는 경험이 불편하더라구요.
    • 그래서, 두 UI를 그려놓고 hide 클래스를 통해 visible을 제어하도록 했습니다.
    • 이렇게 구현하긴 했지만 너무 별로인 거 같아요 ㅜㅜ

질문

현재 루트 /에 있을 때(= 보여줄 Editor가 없을때), 해당 부분을 어떻게 처리하셨나요?

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.

멘토님 말씀처럼 페이지 경로별로 라우팅 처리하는 게 좋을 거 같은데요, 하나의 페이지에서 두 개의 컴포넌트로 분기처리 하면 저라면 innerHTML 사용해서 화면을 그려줄 거 같아요ㅎㅎ 그런데 Editor에서 문서 수정 시 화면이 왜 다시 그려지는 건가요?? 혹시 낙관적 업데이트로는 처리하셨나요?? 👀

Copy link

@yezyvibe yezyvibe left a comment

Choose a reason for hiding this comment

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

수화님 과제 하시느라 고생 많으셨어요ㅎㅎ
문서 트리도 예쁘게 잘 만들어 주셨네요!
404에러 페이지 등 여러 가지 시도하신 게 보여서 좋았습니다👏👏👏

Comment on lines +42 to +56
onDelete: async (id) => {
const targetDocument = await request(`/${id}`);
const removeIdList = getRemoveIdList([targetDocument]);
if (confirm(`하위문서를 포함해 ${removeIdList.length}개가 삭제됩니다`)) {
await Promise.all(
removeIdList.map((id) =>
request(`/${id}`, {
method: 'DELETE',
})
)
);
deleteIsOpenState(removeIdList);
await fetchDocumentList();

if (removeIdList.includes(this.state.currentDocumentId)) {

Choose a reason for hiding this comment

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

한번에 삭제라는 추가 기능도 구현해주셨네요👍👍 개인적인 의견으로 사용자 입장에서 하위 문서는 유지하고 싶은 경우가 있을 거 같아서 사용자에게 모두 삭제 또는 해당 문서만 삭제 이런식으로 선택 사항을 주어도 좋을듯 싶어요 :)
그리고 저도 오프 멘토님과 동일하게 현재 선택한 문서의 하위 문서들의 배열을 저장해두었다가 삭제 처리 시 가져와 사용할 거 같습니다.

import { request } from './utils/api.js';
import { deleteIsOpenState, getRemoveIdList } from './utils/utils.js';

export default function NotionApp({ $container }) {

Choose a reason for hiding this comment

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

이번에는 함수형으로 시도하셨네요 여러 가지로 시도해보시는 거 좋은 거 같아요! 👏👏


if (removeIdList.includes(this.state.currentDocumentId)) {
const hasDocumentList = this.state.documentList.length > 0;
const nextRoute = hasDocumentList ? `/documents/${this.state.documentList[0].id}` : `/`;

Choose a reason for hiding this comment

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

인덱스로 첫 번째 문서의 정보를 가져올 때 매직넘버를 상수화 해서 의미를 담아 사용하면 어떨까요ㅎㅎ


this.currentDocumentId = initialState;

this.setState = async (nextDocumentId) => {

Choose a reason for hiding this comment

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

setState 내 로직이 많은 거 같아서 따로 함수로 분리해주셔도 좋을 거 같습니다ㅎㅎ

Copy link

@zerosial zerosial 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 +29 to +38
onAdd: async (parentId) => {
const createDocument = await request('/', {
method: 'POST',
body: JSON.stringify({ title: '', parent: parentId }),
});
history.pushState(null, null, `/documents/${createDocument.id}`);
this.setState({
...this.state,
currentDocumentId: createDocument.id,
});

Choose a reason for hiding this comment

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

p5) 앗 적어놓고 보니 이미 오프님도 리뷰를 남겨주셧내요
pushState 하는 부분을 함수로 분리해주면 어떨까 싶어요! 여러번 반복되는것 같습니다. 😏


const documentContent = await request(`/${nextDocumentId}`);
if (!documentContent) {
onError();

Choose a reason for hiding this comment

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

p5) 👍

});
e.target.classList.toggle('clicked');
break;
case 'add':

Choose a reason for hiding this comment

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

p5) if문이 많아지면 케이스로 분류한 게 너무 좋습니다. 다만 케이스 안에 if가 또 있고 다시 case가 중첩되는 과정에서 복잡해지는거 같습니다. 아래처럼 함수로 분리 가능하다면 조금 찢어주시는게 좋을 것 같아요 😆

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