[Mission5/신호원] Project_Notion_Vanilla JS 과제 #45
Conversation
setState는 불변성 지키고 validation은 한줄로 쓸 수 있도록 변경
There was a problem hiding this comment.
호원님, 프로젝트 하시느라 정말 수고 많으셨습니다..!
스스로를 민달팽이라고 하시던데, 민달팽이 치고는 기능 구현능력이 뛰어나신 것 같습니다ㅎㅎ
코드 리뷰를 조금 늦게 시작했는데, 하루만에 코드리뷰를 끝내야 하다보니
시간이 없어서 호원님의 코드를 꼼꼼하게 리뷰해드리지 못한 점 죄송합니다ㅠㅠ
개인적으로 호원님의 코드를 읽으면서 전반적으로 좋았던 점은, 항상 코드의 성능 효율성을 위해 새로운 시도를 하시려는 부분이었습니다. 그리고 조금 아쉬웠던 점은 그러한 호원님의 코드 로직이 너무 복잡하다보니 코드를 읽고 이해하기가 어려웠다는 점이었습니다.
데이터가 한 방향으로만 흐르도록 하는 컴포넌트 패턴의 규칙을 최대한 따르면서, 각 컴포넌트는 최대한 하나의 일만 하도록 코드를 작성하는 연습을 해보시는 것도 좋을 것 같다고 생각합니다 :)
Q. 현재 구조 중에 비효율적인 부분이 있는 지 궁금합니다.
시간 관계상 코드를 꼼꼼하게 뜯어보지 못해서 비효율적인 부분이 있는지는 제대로 확인하지 못했습니다ㅠㅠ 다만, 사이드바의 Drawer 컴포넌트에서 render plan 부분이 비효율적이라기보다는, Drawer 의 state 와 Drawer item 의 state 를 sync 를 맞춰주기 위한 로직 코드가 너무 복잡해서 이해하기가 어려웠던 것 같습니다. 이 부분은 성능상으로는 비효율적이더라도, 다시 랜더링해주도록 코드를 작성하는 것도 코드 이해측면에서 좋을 것 같다고 생각합니다.
Q. Sidebar의 DrawerItem 에서 상태 업데이트와 클릭을 위해 커스텀 이벤트와 이벤트 리스너를 많이 사용하고 있습니다. 혹시 문제가 될 지 의견 부탁드립니다.
호원님의 방식은 Drawer > Drawer Item > Drawer > DrawerItem 이런식으로 DrawerItem 을 Drawer 가 감싸주는 방식으로 이해했는데, 여기서 Drawer �Item 마다 이벤트 리스너를 달아주는 것이 문제가 될 수 있을 것 같긴 합니다. 그래서 개인적으로는 호원님이 DrawerItem 에서 처리해주신 이벤트위임 코드의 의미가 퇴색될 것 같다고 생각합니다.
Sidebar 에서 DrawerItem 들의 이벤트 위임을 해주도록 바꿔주시면 더 좋을 것 같다고 생각합니다.
Q. 로컬 스토리지 등의 도움없이 Sidebar의 상태 업데이트 시 open 상태를 유지하기 위해 부분적 렌더링을 구현했습니다. 혹시 로직에 오류가 없는 지 궁금합니다.
아하, 그런 이유로 rendering plan 코드가 탄생한 것이었군요. 저는 충분히 좋은 고민이었고, 그 고민에 대한 해결책을 위해 호원님께서 최선을 다하셨다고 느꼈습니다. 하지만, 제 생각에는 이 부분은 로직의 오류에 대한 고민보다는 좀 더 Component 패턴을 최대한 응용하여 가독성있는 코드를 작성하는 것이 더 좋을 것 같다고 생각합니다!
Q. 빌드를 production으로 하면 문제가 생겨서 development mode로 배포했습니다. 원인을 아신다면 조언 부탁드립니다..
혹시 이 부분은 어떤 문제가 발생하셨는지 공유 부탁드려도 될까요?
src/components/App.js
Outdated
| registerStateSetter($sidebar); | ||
| patchSidebarState(); |
There was a problem hiding this comment.
registerStateSetter() 와 patchSidebarState() 함수는 어떤 이유로 사용하지는건지 궁금합니다!
제 생각에는 각 컴포넌트의 setState 함수를 전역에서 가지고 있다가, 호출하여 사용하는 방법이 조금 어색한 것 같습니다...! 이런 방식으로 setState 함수를 사용하면 코드가 짧아진다는 장점이 있으나, 다른 개발자가 읽기에는 직관적이지 않고, 어떤 컴포넌트에서든 다른 컴포넌트의 setState 를 호출하여 사용할 수 있다는 점에서 휴먼에러가 발생하기 쉬울 것 같습니다
There was a problem hiding this comment.
document가 헤더나 사이드바의 상태 업데이트를 시켜줘야 하다보니 나름대로의 store? 를 구현해보려고 한 건데 데이터 흐름이 직관적이지 않게 된 것 같네요.
src/components/Document/Document.js
Outdated
| <section class="document-title-section"> | ||
| <textarea name="title" value="${this.state.title}"></textarea> | ||
| </section> | ||
| <section class="document-content-section"> | ||
| <textarea name="content"></textarea> | ||
| </section> |
There was a problem hiding this comment.
title 과 content 를 각각 다른 Component 로 분리하고, 각각의 이벤트리스너도 각 Component 에서 관리하도록 하는 방법도 좋을 것 같습니다!
There was a problem hiding this comment.
라고 썼는데, 지석 멘토님의 코멘트와 겹치는 내용이 있는거 같아서 첨언해 보자면, 아래의 quertSelectorAll 을 통해서 title 과 content 에 각각 이벤트 리스너를 달아주고, state 를 init 해주는 로직이 있는 것으로 보이는데, 한 함수 내에서 너무 많은 일을 하고 있는 것처럼 느꼈기 때문입니다.
지석멘토님의 조언처럼, 함수를 쪼개서 명시적으로 보이게끔 리팩토링하는 방법도 괜찮을 것 같구요, 제 경우에는 이런 경우 컴포넌트를 쪼개고 각 컴포넌트의 props 로 eventHandler 를 전달하는 방식을 선호합니다.
제 방식대로 리팩토링한다면... 약간 이런 느낌이 될 것 같네요.
const handleEditTitle = () => { ... };
const handleEditContent = () => { ... };
const $title = new Title({
$target,
initialState: { title: '제목' },
onEdit: handleEditTitle
});
const $content = new Content({
$target,
initialState: { content: '내용' },
onEdit: handleEditContent
});
this.setState = (nextState) => {
const { title, content } = nextState;
$title.setState({ title });
$content.setState({ content });
};There was a problem hiding this comment.
사실 나중에 editor 구현할 때 구체화하겠거니 하고 적당히 채워넣은 부분이라 뭉뚱그려진 느낌이 나버렸네요..
팀 스터디 때 개선해보겠습니다!
| this.render(); | ||
| }; | ||
|
|
||
| this.init = once(() => { |
There was a problem hiding this comment.
once 를 사용하여 한 번만 실행되도록 할 수 있군요!
호원님의 코드에서 좋은 것을 배워갑니다 ㅎㅎ
src/components/Sidebar/Drawer.js
Outdated
| this.renderingPlan.forEach((plan) => { | ||
| if (plan === 0) { | ||
| this.children[stateIdx].setState(this.state[stateIdx]); | ||
|
|
||
| $currentNode = $currentNode.nextSibling; | ||
| stateIdx += 1; | ||
| } else if (plan > 0) { | ||
| const $drawerItem = new DrawerItem({ | ||
| $target: $drawer, | ||
| $sibling: $currentNode, | ||
| parent, | ||
| level, | ||
| }); | ||
| $drawerItem.setState(this.state[stateIdx]); | ||
| this.children.splice(stateIdx, 0, $drawerItem); | ||
|
|
||
| stateIdx += 1; | ||
| } else { | ||
| const $tempNextNode = $currentNode?.nextSibling; | ||
| $drawer.removeChild($currentNode); | ||
| $currentNode = $tempNextNode; | ||
|
|
||
| this.children.splice(stateIdx, 1); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Drawer 에서 각각의 컴포넌트의 render plan 을 하나의 배열로 가지고 있고, 이 plan 에 따라서 rendering 해주는 방법인 것으로 이해했습니다.
setRenderingPlan 으로 미리 랜더링 계획을 계산해주도록 하는 부분은 좋은 시도였던 것 같으나, 저는 개인적으로 호원님의 해당 코드를 이해하기가 조금 어려웠던 것 같습니다. 또한, 각각의 DrawerItem 의 상태를 현재 rendering plan 과 동일하게 싱크를 맞춰주어야 하기 때문에 호원님의 코드에 다른 기능을 추가하거나 수정하기가 어려울 것 같다는 생각이 듭니다.
제 생각에는 모든 DrawerItem 을 그냥 처음부터 다시 생성, 랜더하도록 하는 편이 가독성 측면에서 더 좋지 않을까 생각합니다..!
There was a problem hiding this comment.
호원님 진짜 너무 고생하셨습니다! 컴포넌트도 잘 분리하시고, 코드도 잘 읽혔습니다!
진짜 노션인줄 알고 깜짝 놀랐습니다....🙄
항상 조용히 최고의 길을 걸으시는.... 저도 호원님처럼 꾸준하게 자신만의 길을 걷는 사람이 되고 싶어요!
몇가지 추가로 말씀드리고 싶은 것이 있는데, 어디에 적어야할지 몰라 여기에 적겠습니다!
-
기본사항 중에
"Document Tree에서 각 Document 우측에는 + 버튼이 있습니다. 해당 버튼을 클릭하면, 클릭한 Document의 하위 Document로 새 Document를 생성하고 편집화면으로 넘깁니다."
부분이 있습니다!
하지만 새 페이지를 생성하였을 때, 생성된 페이지로 넘어가지 않습니다!
호원님께서 이미 생성해두신 함수routeToDocument를 활용하면 금방 수정하실 수 있을 것 같습니다! -
상위 문서에서 하위 문서를 생성할 때, 1-2초간 상위 문서 이름이 '제목 없음'으로 변경되는 오류가 있습니다! 이 부분도 수정해주시면 더욱 좋은 UX가 될 것 같습니다!
너무 수고하셨습니다
src/components/Document/Document.js
Outdated
| } | ||
|
|
||
| let timer; | ||
| const $document = document.createElement("section"); |
There was a problem hiding this comment.
시멘틱 태그를 적절하게 잘 사용하시는 것 같아요!!!
section 태그는 섹션(부분, 구역, 영역) 들을 그룹화 해서 분리하는 역할을 한다고 합니다! 참고
저는 그냥 무작정 div로 하는데 고쳐야 될 것 같습니다😓
src/components/Document/Document.js
Outdated
| this.render(); | ||
| }; | ||
|
|
||
| this.init = once(() => { |
There was a problem hiding this comment.
once란 함수이름으로 단 한번만 실행된다는 것을 바로 알았습니다 ㅎㅎ
네이밍 센스👍
| if (!isConstructor(new.target)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
유효성 검사를 진짜 꼼꼼히 하신 것이 느껴집니다!
배우겠습니다!
src/components/Home/Home.js
Outdated
| $recent.className = "home-recent-container"; | ||
| $most.className = "home-most-container"; |
There was a problem hiding this comment.
최근 사용한 문서와, 가장 많이 사용한 문서를 보여주고, 클릭시 해당 페이지로 이동하는 기능 너무 멋져요~!!
저도 기회가 되면 추가해 보고 싶습니다!
src/components/Home/Home.js
Outdated
| .sort( | ||
| ([aid, aval], [bid, bval]) => bval.lastUsedTime - aval.lastUsedTime | ||
| ) | ||
| .slice(0, 10) | ||
| .map(([id, { title }]) => `<p data-id=${id}>${title}</p>`) | ||
| .join("")} |
There was a problem hiding this comment.
크~ 알고리즘 문제를 매우 깔끔하게 푸시더니 역시 빛을 발휘하시네요👏
There was a problem hiding this comment.
sort, slice, map, join 다 저희가 머쓱이 하면서 많이 배운 부분인데 과제에서 빛을 발하시는 호원님 ㅎㅎ
능숙하게 사용하시는 모습 존경합니다! 저도 배우고 싶어요 ㅎㅎ
src/components/Sidebar/DrawerItem.js
Outdated
| window.removeEventListener("route-drawer", this.activate); | ||
| window.removeEventListener("title-updated", this.updateTitle); |
There was a problem hiding this comment.
이 부분은 Item을 삭제하면 저절로 사라지지 않을까요?
따로 이벤트를 삭제해주는 부분을 추가해주신 이유가 궁금합니다!
There was a problem hiding this comment.
이 부분은 삭제하는 부분인거 같은데 삭제한 뒤의 결과물? 상태값을 result에 넣어주는 건가요?
result 보다는 좀 더 삭제가 정상적으로 되었다 또는 비정상적으로 되지 않았다는 의미를 담은 변수명을 지어주면 어떨까요?
delete에 res를 받는 부분인 거 같아서요.
There was a problem hiding this comment.
명시적으로 삭제해주지 않으면 메모리에 남아서 나중에 터지기 때문에 이벤트 위임을 쓴다는 글을 본 것 같아서 추가했습니다!
result는 확실히 어색한 것 같아서 response로 바꿨습니다.
src/utils/Errors.js
Outdated
| export const ERROR = { | ||
| NEW_MISSED: "Current component is declared without 'new' keyword.", | ||
| NONE_OBJECT_STATE: "Tried to set state using none object value.", | ||
| INVALID_DOCUMENT_STATE: "Tried to set document's state using invalid data.", | ||
| INVALID_DRAWER_STATE: "Tried to set drawer's state using invalid data.", | ||
| INVALID_DRAWERITEM_STATE: | ||
| "Tried to set drawer item's state using invalid data.", | ||
| INVALID_HEADER_STATE: "Tried to set header's state using invalid data.", | ||
| INVALID_HOME_STATE: "Tried to set home's state using invalid data.", | ||
| NONE_ARRAY_STATE: "Tried to set state using none array value.", | ||
| INVALID_REQUEST: "서버에 문제가 발생했습니다.\n 잠시 후 다시 시도해주세요.", | ||
| }; |
src/components/Document/Document.js
Outdated
| $document.className = "document-container"; | ||
| $document.innerHTML = ` | ||
| <section class="document-title-section"> | ||
| <textarea name="title" value="${this.state.title}"></textarea> |
There was a problem hiding this comment.
title에 textarea를 사용하셔서 신기했어요.
여러 줄 입력을 고려해 input 대신 textarea를 선택하신 걸까요?
There was a problem hiding this comment.
임시로 대충 때웠던 흔적입니다 🥲
스터디 때 수정해보겠습니다.
src/components/Home/Home.js
Outdated
| import { stateSetters } from "@Utils/stateSetters"; | ||
|
|
||
| export default function Home({ $target }) { | ||
| if (!isConstructor(new.target)) { |
There was a problem hiding this comment.
이번에 저는 new 방어코드를 신경쓰지 못했는데 신경써서 수정해야겠습니다!
src/components/Home/Home.js
Outdated
| .sort( | ||
| ([aid, aval], [bid, bval]) => bval.lastUsedTime - aval.lastUsedTime | ||
| ) | ||
| .slice(0, 10) | ||
| .map(([id, { title }]) => `<p data-id=${id}>${title}</p>`) | ||
| .join("")} |
There was a problem hiding this comment.
sort, slice, map, join 다 저희가 머쓱이 하면서 많이 배운 부분인데 과제에서 빛을 발하시는 호원님 ㅎㅎ
능숙하게 사용하시는 모습 존경합니다! 저도 배우고 싶어요 ㅎㅎ
src/components/Home/Home.js
Outdated
|
|
||
| stateSetters[NAME.HEADER]([]); | ||
| }; | ||
| } |
There was a problem hiding this comment.
최근 사용한 문서와 자주 사용한 문서 기능을 추가하셔서 유저 입장에서 더 좋은 것 같아요 💯
src/components/Sidebar/DrawerItem.js
Outdated
| window.removeEventListener("route-drawer", this.activate); | ||
| window.removeEventListener("title-updated", this.updateTitle); |
There was a problem hiding this comment.
이 부분은 삭제하는 부분인거 같은데 삭제한 뒤의 결과물? 상태값을 result에 넣어주는 건가요?
result 보다는 좀 더 삭제가 정상적으로 되었다 또는 비정상적으로 되지 않았다는 의미를 담은 변수명을 지어주면 어떨까요?
delete에 res를 받는 부분인 거 같아서요.
src/utils/Errors.js
Outdated
| INVALID_HOME_STATE: "Tried to set home's state using invalid data.", | ||
| NONE_ARRAY_STATE: "Tried to set state using none array value.", | ||
| INVALID_REQUEST: "서버에 문제가 발생했습니다.\n 잠시 후 다시 시도해주세요.", | ||
| }; |
There was a problem hiding this comment.
validation과 error 처리 부분 깔끔하게 정리해주시고 세심하게 챙기시는 모습 저도 본받아야 겠네요...
이번 과제에서는 신경쓰지 못했는데 배워갑니다 ㅎㅎ
| return await request(`/documents/${documentId}`, { | ||
| method: "DELETE", | ||
| }); | ||
| } |
There was a problem hiding this comment.
api를 정리해서 함수화한 코드가 더 사용하기도 좋고, 깔끔한 거 같아요!
저는 그때그때마다 작성했는데 이런 부분은 수정하는 편이 더 좋을 것 같네요.
항상 배워갑니다.
| new ValidationError(ERROR.INVALID_HOME_STATE) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
꼼꼼하게 validation 신경써주시는 모습 좋습니다 💯
|
@arclien @kutta97 답글을 남길 곳이 없어서 production 배포 이슈는 여기에 올립니다. 이 부분에서 stateSetters[NAME.HOME] 이 function이 아니라는 오류가 납니다. 마지막 커밋에 base url 수정은 혜진님과 창욱님꺼 세팅 보면서 수정한 부분입니다. 해당 문제가 해결되지는 않더라구요. |
eslint error에 맞춰 수정함
setStateOf으로 감싸고 에러 처리 추가
DocumentContainer에서 route 관리하도록 분리
배포용 레포 코드 머지 용
dispatch와 autoSave로 함수 분리
openParents와 sendHeaderDocsInfo를 activate 내부에서 통합 처리
click 핸들러 내부에서 action에 따른 로직 핸들러로 분리
마진 제거 및 커서 설정
삭제 문서 replaceState 대응
📌 과제 설명
노션 클로닝 프로젝트 🗒️
<a>배포 링크</a>
레포지토리를 클론한 뒤
yarn&yarn start를 실행하면 webpack-dev-server로 실행할 수 있습니다.Vanilla JS 로 작성하였고 각 js, html, css, svg 파일은 webpack 의 loader module을 통해 빌드하였습니다.
컴포넌트 구조
App
App 컴포넌트에서 Sidebar 와 Header를 렌더링하고 router를 설정합니다.
router
router 함수에서 url 을 기준으로 id에 맞는 Document나 Home 페이지를 렌더링합니다.
Sidebar
Sidebar는 root Document의 링크 묶음인 root Drawer를 가지고 있습니다.
Drawer는 Document의 링크인 DrawerItem 요소를 묶습니다.
DrawerItem은 하위 요소로 Drawer를 다시 가져 자신의 자식 문서 링크를 렌더링합니다.
이를 통해 트리 구조의 Document 링크들을 계속해서 렌더링할 수 있습니다.
Header와 Sidebar의 state는 state setter를 별개의 파일은 stateSetter.js에 등록하여 Document 컴포넌트에서도 state 조작이 가능하게 하였습니다.
👩💻 요구 사항과 구현 내용
기본 요구사항 구현 내용
추가 구현 내용
구현 미완료
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점