-
Notifications
You must be signed in to change notification settings - Fork 28
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 #16
base: 3/#4_minjae
Are you sure you want to change the base?
[Mission4/김민재] - Project_Notion_VanillaJS #16
Conversation
API url을 상수로 선언하고 해당 내용을 gitignore로 업로드 하지 않는 방식을 사용했습니다.
필수적인 파일들과 CSS 추가
SPA를 위한 router, validation을 검사하는 함수를 추가
처음 한번만 문서리스트를 받아오고 url이 바뀔 때 문서 내용을 가져오는 역할을 합니다.
초기나 오류 발생시 화면 처리를 위한 랜딩페이지를 구현
state는 문서 리스트로 관련된 내용들을 다루려고 노력하였습니다.
문서 id 값을 App 컴포넌트에서 state를 통해 가져와 그 값을 활용하려고 노력하였습니다.
초기 페이지 docId가 undefined도 함께 방어하는 코드 작성
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 민재님!
첫번째 개인 프로젝트 고생 많으셨습니다.
컴포넌트간의 의존성을 줄이기 위해서 노력하신 모습이 보이네요
이런저런 시도를 많이 하셨던 만큼 배워가는게 많은 프로젝트였으면 좋겠습니다!
src/components/sidebar/DocList.js
Outdated
$newSubDoc.innerHTML = ` | ||
<p class="forHover"> | ||
<button class="toggleFold">►</button> | ||
<span class="docTitle">제목 없음</span> | ||
<span class="controlBtns"> | ||
<button class="newSubDoc">➕</button> | ||
<button class="delete">X</button> | ||
</span> | ||
<ul class='child' style='display: none;'><li class="isEnd">하위 페이지가 없습니다.</li></ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분에서 하위리스트가 삭제 되면 상위 리스트의 토글상태도 초기화 되는것으로 보입니다.
이렇게 될경우 사용자가 하위부터 상위로 올라가면서 리스트를 삭제할때 계속해서 토글버튼을 누르면서
삭제해야할 것 같습니다.
---root
---1
--- 1-2
문서 1-2
를 삭제시 문서 1
를 지우기위해서는 문서 root
의 토글버튼을 한번 더 눌러야함
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 따로 트리토글 저장버튼이 없어서 이를 어떻게 처리할지 고민을 많이 한 부분인데 api 처리가 실패하지 않는다면 사이드바의 문서가 변경되지 않도록 처리하였습니다. 놓친 부분인데 리뷰해주셔서 감사합니다!.
const $curLi = document.getElementById(id); | ||
const $ul = $curLi.closest("ul"); | ||
|
||
if ($ul.className === "child") { | ||
path.push([$curLi.querySelector("span").innerHTML, id]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const $curLi = document.getElementById(id); | |
const $ul = $curLi.closest("ul"); | |
if ($ul.className === "child") { | |
path.push([$curLi.querySelector("span").innerHTML, id]); | |
const $curLi = document.getElementById(id); | |
if ($curLi) { | |
const $ul = $curLi.closest("ul"); | |
if ($ul.className === "child") { | |
path.push([$curLi.querySelector("span").innerHTML, id]); | |
$curLi가 존재하는가에 대한 방어코드를 넣는것도 좋을듯 합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 컴포넌트에서는 방어코드가 없었는데 중요한 부분을 잘 잡아주신것 같습니다. 리뷰 반영해보도록 하겠습니다! 감사합니다👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 민재님, 노션 클로닝 과제도 수고하셨습니다!
노션 클로닝 과제를 끝으로 팀이 바뀌네요.
과제를 하시면서 여러 가지 시도를 하시는 모습을 보고 많이 배웠습니다!
time, | ||
mark, | ||
audio, | ||
video { | ||
margin: 0; | ||
padding: 0; | ||
border: 0; | ||
font-size: 100%; | ||
font: inherit; | ||
vertical-align: baseline; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
선택자 하나하나 선언해서 스타일을 적용하셨는데 *
전체 선택자를 사용하면 퍼포먼스의 저하가 있다고 해서 이렇게 적용하신 거였나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 경우는 reset을 위해 특성들을 초기화해주는 정리된 코드를 복붙한 경우입니다!
https://meyerweb.com/eric/tools/css/reset/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
민재님 프로젝트 수행하시느라 고생하셨습니다.
여러가지 내용들을 신경써서 작성하신 것 같습니다!
리뷰를 진행하면서 저도 제 결과물에 대해서 더 생각해보게 되었습니다.
고생하셨습니다~!
.gitignore
Outdated
@@ -0,0 +1,2 @@ | |||
constants.js | |||
storage.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스토리지도 이그노어 하신 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번 프로젝트에서 storage를 사용하지 않아서 ignore했다가 지우는걸 잊었습니다.😓 리팩토링 진행하며 storage를 사용해볼 생각입니다.
|
||
this.state = initialState; | ||
|
||
this.setState = (nextState) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state도 validation을 거치면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 validation 적용해보도록 하겠습니다. 감사합니다!
|
||
const $breadCrumb = document.createElement("nav"); | ||
$breadCrumb.className = "linkContainer"; | ||
$target.appendChild($breadCrumb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엘리먼트의 내용을 작성하고 나서 append를 시키는 것도 좋아 보입니다. 커피챗 때 그렇게 해야 DOM의 조작이 줄어든다고 들었던 것 같습니다.
src/components/editpage/Editor.js
Outdated
|
||
$editor.addEventListener("keyup", (e) => { | ||
const targetTag = e.target.tagName; | ||
const { value } = e.target; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.target을 공통적으로 사용하므로 const {target} = e 로 꺼내서 사용할 수 있겠네요.
src/router.js
Outdated
|
||
export const push = (nextUrl) => { | ||
window.dispatchEvent( | ||
new CustomEvent("route-change", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 "route-change"를 상수로 선언해 두었으므로 그것을 사용하는 것이 좋겠네요.
수정 하는걸 깜빡하신듯 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
깜빡하고 수정하지 않았네요😓 수정하겠습니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
민재님, 프로젝트 하시느라 고생많으셨습니다. 디코방에서 질문 같은거 있을 때 활발히 소통해주셔서 감사했습니다.천천히 읽어보도록 하겠습니다!
저랑 뭔가 구조적으로 비슷한 부분(?)이 있어서 그런지 개인적으로 따라가기 쉬운 코드를 짜주셨다는 생각이 드네요. 수고하셨습니다
style.css
Outdated
} | ||
|
||
.linkContainer { | ||
color: rgba(25, 23, 17, 0.6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
css로 js의 변수처럼
:root {
--white100: #fff;
}
다음과 같이 선언하여
var(--white100)
이렇게 사용할 수 있습니다. 중복되어 사용되는 색상의 같은 경우 이런식을 관리해 주어도 괜찮을 거 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전에 한번 사용해본 내용인데 이 경우에 적용하니 오히려 보기 편하네요! 리뷰 감사합니다! 수정하였습니다.
src/components/editpage/SubLink.js
Outdated
$subLink.addEventListener("click", (e) => { | ||
const { id } = e.target.closest("div").dataset; | ||
const $curLi = document.getElementById(id); | ||
const $parentUl = $curLi.closest("ul"); | ||
|
||
$parentUl.closest("li").querySelector(".toggleFold").innerText = "▼"; | ||
$parentUl.style.display = "block"; | ||
clickLink(id); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
크기가 작고 컴포넌트이고 클릭되는 것이 제한되어 있지만, 그래도 나중에 코드가 추가되는 경우를 생각하여(지금은 그럴일은 없지만😂) 방어코드가 있으면 조금 더 좋지 않을까 싶습니다
onClick: () => { | ||
push("/"); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 컴포넌트의 자원을 사용한다거나, 자식 컴포넌트별로 다른 작업을 하는 것이 아닌, push("/")
만을 위한 함수라면 굳이 상위 컴포넌트에서 작업할 이유는 없다고 생각해서 일단 +눌렀는데....해당 위치에서만 push
가 이루어지도록 의도한 것이라면 구조적으로는 괜찮은 거 같네요ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정확하게 봐주셨습니다😄 굳이 하위에서 또 import하지 않고 sidebar에서만 push를 사용해보려고 하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state를 크게 두개로 나눠 다루려고 했는데(문서 리스트, 문서 내용) 효율적이지 못한 방법일까요?
각 컴포넌트에서 필요한 데이터를 단락으로 나누어 전달하는 접근법은 좋았다고 생각합니다.
문서리스트를 너무 자주 fetch하지 않으려고 필요한 상황에서만 fetch를 진행하였으나 이로인해 DOM 조작을 과도하게 사용하게 되었습니다. DOM 조작은 최소화하고 데이터를 자주 가져오더라도 데이터 중심적으로 구현하는게 좋을까요?
DOM 조작과 API 호출 중 비용이 더 높은 건 API 호출입니다. DOM 조작이 다소 발생하더라도 API 호출을 줄일 수 있다면 그 편이 더욱 이득이라고 생각합니다. (물론 둘 다 줄일 수 있으면 좋겠죠)
parent Id 값이 따로 정리되어 있지않아 BreadCrumb 컴포넌트를 구현할 때 DOM과 재귀로 최상위를 찾는 방식을 사용했는데 좋지 못한 방식일까요?
주어진 상황에서(API에서 반환되는 데이터 형식 등) 문제를 해결하기 위한 적절한 선택이었다고 생각합니다.
contenteditable을 이용하려고 할 때 상태 변경이 일어나면 커서 포커스가 앞으로 이동하여 구현에 어려움을 겪었습니다. 검색 결과 커서의 위치를 저장해뒀다가 상태 변경후에 해당 위치로 보내는 방식을 사용한다고 하였는데 이 방식 말고도 다른 방식이 있을까요?
데이터 상태 기반으로 content를 다루는 경우에는 직접 커서의 위치도 컨트롤 해주는 방식으로 접근하는 게 맞습니다.
대부분의 오류가 상태값이 예상한 값이 아닐 때 일어나 방어 코드를 적는데 많은 시간을 소비했습니다. 완벽한 방어코드를 작성했다는 생각이 들지 않는데 방어 코드를 작성하는 꿀팁이 있을까요?🥲
검증은 항상 '수준'의 문제라고 말씀드렸던 것 같은데요. 완벽한 코드란 존재할 수 없다고 생각합니다. 애초에 인간이라는 존재가 완벽하지 않으니까요.
그래서 사실 '소프트웨어 테스팅'이라는 별도의 학문에서 이를 전문적으로 다르기도 하고, 개발자 입장에서는 가장 치명적일 것으로 예상되는, 문제가 빈번하게 발생될 것으로 예측되는, 사용자 경험에 해가 될 것으로 추측되는 케이스를 위주로 고려해보면 좋을 것 같습니다.
아, 물론 위 가정에서 마가렛 해밀턴은 예외입니다. (...)
index.html
Outdated
<title>Minjae의 Notion</title> | ||
</head> | ||
<body> | ||
<main id="app" style="display: flex; flex-direction: row"></main> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 인라인 스타일로 지정한 이유가 무엇인가요? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초반에 레이아웃을 보기위해 설정해놓고 잊고 있었습니다..🥲 리뷰 감사합니다~! 수정하겠습니다.
style.css
Outdated
button { | ||
color: rgba(25, 23, 17, 0.6); | ||
border: 0; | ||
padding: 2px; | ||
background-color: transparent; | ||
cursor: pointer; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초기화 목적이 아닌 스타일을 부여하는 경우는 기본 엘리먼트의 속성을 덮어쓰기보다는 가급적 클래스를 통해 정의하는 편이 용이합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
미처 생각하지 못한 부분인데 바로 잡아주셔서 감사합니다! 수정했습니다!
src/validation.js
Outdated
if (!target) | ||
throw new Error(`${component} 컴포넌트는 생성자 함수입니다. new 키워드를 추가해주세요.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: 저는 조건 분기에 실행되는 코드가 한 줄이라도 괄호를 사용하는 편인데, 괄호를 사용하지 않는 경우엔 보통 한 줄로 이어서 처리하는 케이스가 많긴 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번 경우에도 한줄로 처리하고 싶었는데 길이가 길어서 내려온 것 같습니다.😓 가독성을 위해서 괄호를 사용하는걸 연습하겠습니다. 리뷰 감사합니다!
this.state = { | ||
documentsList: [], | ||
editorDocument: { | ||
docId: null, | ||
doc: { | ||
title: "", | ||
content: "", | ||
documents: [], | ||
}, | ||
}, | ||
}; | ||
|
||
const sidebar = new Sidebar({ | ||
$target, | ||
initialState: this.state.documentsList, | ||
}); | ||
|
||
new LandingPage({ $target }); | ||
|
||
const editContainer = new EditorContainer({ | ||
$target, | ||
initialState: this.state.editorDocument, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial state 관리 방식 좋네요 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다!🙇🏻♂️ 필요한 state들만 사용해보려고 노력해봤습니다.
src/api.js
Outdated
import { API_END_POINT } from "./constants.js"; | ||
import { X_USERNAME } from "./constants.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동일한 경로의 모듈에서 불러오는 바인딩은 한 줄로 처리해도 될 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
미처 확인하지 못했습니다.. 리뷰 감사합니다!
return path | ||
.reverse() | ||
.map((el) => `<span class="link" data-id=${el[1]} >${el[0]}</span>`) | ||
.join(" / "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/components/editpage/SubLink.js
Outdated
if (doc.documents.length > 0) { | ||
$subLink.innerHTML = doc.documents | ||
.map((el) => `<div class="link" data-id=${el.id}>${el.title}</div>`) | ||
.join(""); | ||
} else { | ||
$subLink.innerHTML = ` | ||
`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 경우는 삼항 연산자도 좋을 것 같네요.
src/components/editpage/Editor.js
Outdated
const targetTag = e.target.tagName; | ||
const { value } = e.target; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const targetTag = e.target.tagName; | |
const { value } = e.target; | |
const { tagName: targetTag, value } = e.target; |
src/components/sidebar/DocList.js
Outdated
? `<ul class='child' style='display: none;'>${renderDocList(documents)}</ul>` | ||
: `<ul class='child' style='display: none;'><li class="isEnd">하위 페이지가 없습니다.</li></ul>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ul의 display 속성을 직접 제어하기보다는 child 클래스의 display 속성에 기본 값으로 none을 주고 display: block 등의 속성이 부여된 child--show
(BEM 네이밍 형식입니다) 클래스를 뗐다 붙였다 하는 식으로 제어를 해보시면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
직접 속성을 제어하며 헷갈리는 경우가 많았는데 리뷰를 반영하며 클래스를 이런 방식으로 정하고 css에 따로 정리하니 코드 작성이 수월해졌습니다. 감사합니다 멘토님!
src/components/sidebar/DocList.js
Outdated
await onNewSubDoc(id); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반환되는 Promise의 값을 뜯어서 직접 접근할 일이 없으면 굳이 await을 사용하지 않아도 됩니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
놓친 부분 바로 잡아주셔서 감사합니다! 수정하겠습니다.
appendChild를 하지 않아 발생한 문제,
사이드바의 dom에 접근하여 background-color를 바꾸고 이전 요소는 지워주는 메소드 작성 및 적용
📌 과제 설명
👩💻 요구 사항과 구현 내용
기본 요구사항으로 제공된 내용은 모두 구현하였고 보너스는 한가지 구현하였습니다.
구현한 기본 요구사항
구현한 보너스 요구사항
미리보기
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
우선 이번 과제를 진행하며 부족함을 많이 느꼈습니다..😔 의존성이 많이 발생했다고 느끼고 있습니다.
초기 계획으로는 문서 리스트를 한번만 불러오고 서버와 통신이 실패하지 않는다면 낙관적 업데이트로 대부분을 해결해보려고 하였으나
이 방법이 생각보다 많이 복잡하고 또 상위 문서를 삭제하여 하위 문서들이 Root로 올라올 때와 같이 업데이트가 필요한 상황들로 어려움을 겪었습니다.
전체적인 구조에 대해서는 무난한 방식으로 진행하였고 크게 두 파트 Sidebar, EditContainer로 구분하여 각 상태마다 렌더링을 다르게 하는 방식으로 진행하였습니다.
Sidebar 부분에서는 문서리스트에 대한 state만을 가져오고 부족한 부분은 url의 id 값을 활용하려고 노력하였습니다.
EditContainer 부분은 url의 id를 fetch하여 그 상태를 사용하려고 노력하였습니다.
전반적으로 계획했던 부분과 많이 다르고 DOM을 과도하게 사용했다고 생각하고 있습니다. 또한 구현하면 좋을 것 같았지만 구현하지 못한 트리 Fold 상태 값이나 비정상적으로 종료시 내용을 불러오는 부분은 리팩토링에서 꼭 구현해보겠습니다.