-
Notifications
You must be signed in to change notification settings - Fork 29
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 과제 #46
base: 4/#5_colorkite10_working
Are you sure you want to change the base?
[Mission5/이채연] Project_Notion_VanillaJs 과제 #46
Conversation
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.
채연님 이번 과제 수행하시느라 너무 고생하셨습니다!😄😄
white-space: nowrap; | ||
height: 100vh; | ||
backgroundColor: #EDECE9; | ||
`; |
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파일에 포함시킨 이유가 있나요?? 👀
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를 아직 공부 중이기 때문에...입니다!!
id: id, | ||
displayed: "none", | ||
}); | ||
} |
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.
하위 document를 펼치는 토글 여부를 로컬 스토리지에 저장하는 방식으로 새로고침했을때 사이드바가 초기화되지 않게금 하셨네요!! 👍👍 저도 이렇게 구현하고 싶었는데 모든 document를 로컬스토리지에 저장하는 방식인데 document가 많아진다면 로컬스토리지에 부담이 가지 않을까 생각을 해서 적용하지 않았는데 어떻게 생각하시나요?
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.
채연님! 이번 노션 과제 정말 수고 많으셨습니다..!
저도 과제가 재밌기도 하면서 아쉬움과 많은 자극을 받고 있습니다!
채연님의 코드를 보면서도 채연님의 생각과 많은 고민을 볼 수 있어서 좋았습니다!
리뷰 이후에도 수정하면서 완전히 채연님 것으로 만들었으면 합니다. 남은 과제에서도 같이 화이팅해요! 🙇♂️
content: "", | ||
}); | ||
|
||
if (tempDocument.title !== "" || tempDocument.content !== "") { |
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.
if (tempDocument.title !== "" || tempDocument.content !== "") { | |
if (!(tempDocument.title === "" && tempDocument.content === "")) { |
똑같이 동작하는 코드인 것 같아서 제안드려봅니다..! 🤔
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.
@jgjgill 제 생각에는 OR
연산자가 더 나은 것 같습니다!! AND
연산자는 2개 다 비교해야하지만 OR
연산자 같은 경우 한 가지의 경우만 만족하면 되기 때문입니다 😀
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.
소소한 TMI
형변환을 통한 검증도 가능할 것 같아요!
if (tempDocument.title !== "" || tempDocument.content !== "") { | |
if (!tempDocument.title || !tempDocument.content) { |
$documentList.addEventListener("click", (e) => { | ||
const $li = e.target.closest("li"); | ||
|
||
if (!$li) 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.
👍👍👍
if (confirm("임시저장된 값을 불러올까요?")) { | ||
this.setState({ | ||
...this.state, | ||
doc: tempDocument, | ||
}); | ||
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.
임시 저장 기능을 구현해서 사용자에게 좋은 것 같아요!! 😆
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.
저도 임시 저장 기능을 구현한 것은 매우 좋다고 생각돼요!!! 👍
다만 위에 적은 것 처럼 fetchDocument
라는 함수에서 기대할 수 있는 행동인지는 살짝 의문이 있어요.
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.
안녕하세요 채연님! 과제 잘 봤습니다!!ㅎㅎ
오류도 수정하시면서 뭔가 최대한 많은 기능들을 구현하시려는 노력이 돋보인 코드였습니다!
저도 컴포넌트형 사고를 하고 싶어서 채연님과 같은 고민을 했었는데요!
괜히 상태관리 라이브러리들이 나온게 아니구나~ 라고 저는 일단 생각하고 있습니다😅
리팩토링된 코드가 정말 기대가 된다~ 라고 생각하면서 코드를 읽어보았습니다! 기대할게요!! 수고하셨습니다!!
} catch { | ||
console.log("페이지가 어디로 갔나봐요. 새로고침을 눌러주세요!"); | ||
} |
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.
P5;
우선 사용자 입장을 생각한 에러 처리 너무 멋지네요~~~! 👍
다만 소소한 우려로 사용자를 위한 에러 문구를 여기서 띄우는 것이 맞을까? 하는 생각이 들었어요.
(추가로 setItem과 페이지가 어디로 갔나봐요 에러는 어떤 상관관계가 있는지 궁금해 졌어요.)
만약 문구를 상황에 따라 다르게 하고 싶다면? console.log가 아닌 다른 방식으로 안내를 하고 싶다면? 등과 같은 상황을 맞이하면 난처 할 것 같거든요.
지금 상황에서는 사용하는 쪽에서 핸들링 할 수 있게 하는 게 좋지 않을까 싶은데 채연님은 어떠신가요?
(이런 경우 사용하는 곳에서 try catch를 하거나 콜백 패턴 등을 사용하기도 해요.)
removeItem(selectedDocId); | ||
removeItem(`temp-document-${selectedDocId}`); |
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.
temp-document-selectedDocId와 selectedDocId 두개를 지워주는군요! 두개의 키가 하는 역할이 다른건가요?!🤔
const documentEditPage = new DocumentEditPage({ | ||
$target, | ||
initialState: { | ||
docId: "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.
이 new라는 부분 때문에 그런건지 모르겠는데, add a document 버튼을 누르면 id가 string 타입으로 들어가고 하위 문서 생성을 하면 id가 number 타입으로 들어가는 것 같은데요! 한 번 확인해보시면 좋을 것 같습니다!
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.
P3;
docId의 ID가 주로 가지는 의미를 생각해보면 "중복되지 않는 유니크한 값" 이라고 생각돼요.
그랬을 때 new라는 값은 이후에 변경될 지언정 적절한지는 살짝 의문이 들었어요.
만약 id 값을 만드는게 막연하다면 uuid 같은 값을 만들어서 부여해보면 어떨까요?
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.
채연님 PR을 보니 과제를 해결하려고 정말 많은 고민을 하셨던 것 같습니다🥹 과제하시느라 수고 많으셨습니다!
저는 데이터 흐름을 파악할 때 컴포넌트들의 state들과 어디서 바뀌는지를 적어봅니다.. 그러면 각 state에 어떤 프로퍼티가 있는지도 덜 헷갈리고, 데이터의 흐름도 조금은 파악하기 쉬워지는 것 같습니다
PR에 적힌 수정할 사항들의 문제 원인은 무엇이었을지 궁금합니다 나중에 해결하시면 알려주세요🔔 코드 구경하러 오겠습니다!
if (name === "displayChild") { | ||
const $ul = $li.childNodes[7]; | ||
|
||
if (!$ul) { | ||
setItem(id, { | ||
id: id, | ||
displayed: "none", | ||
}); | ||
push(`/documents/${id}`); | ||
return; | ||
} | ||
|
||
if ($ul.style.display === "") { | ||
$ul.style.display = "none"; | ||
setItem(id, { | ||
id: id, | ||
displayed: "none", | ||
}); | ||
} else { | ||
$ul.style.display = ""; | ||
setItem(id, { | ||
id: id, | ||
displayed: "", | ||
}); | ||
} | ||
push(`/documents/${id}`); | ||
} else if (name === "title") { | ||
push(`/documents/${id}`); | ||
} else if (name === "add") { | ||
onCreateDocument(id); | ||
fetchDocument(); | ||
} else if (name === "delete") { | ||
onDeleteDocument(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.
p5; if-else문으로도 충분하지만 switch문으로 바꾸면 name에 대한 다양한 상황을 좀 더 한눈에 볼 수 있을 것 같습니다 사소한 의견입니다!
${this.displayDocumentList(doc.documents)} | ||
</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.
어떻게 트리 구조를 구현하셨을까 궁금했는데 재귀함수를 사용하셨군요! 제게는 아직 재귀를 사용한 구현은 어렵네요..!
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.
채연님!! 과제 하느라 고생하셨습니다. 새벽에 과제를 함께 하면서 서로 sidebar 하위 document를 어떻게 구현할지 고민했던 기억이 나네요!! 앞으로도 함께 잘 해쳐나가면 좋겠습니다! 감사합니다! 👏🙋🏻♂️😁
$target.appendChild($linkButton); | ||
this.render = () => { | ||
$linkButton.textContent = this.state.text; | ||
}; | ||
this.render(); |
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.setState = (nextState) => { | ||
this.state = nextState; | ||
documentPage.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.
documentPage.setState(nextState); | |
documentPage.setState(this.state); |
this.state
가 nextState
값이기 때문에 setState(this.state)
가 적절할 것 같습니다!! 😁
const [, , currentPageId] = pathname.split("/"); | ||
|
||
if (selectedChildIds.includes(Number(currentPageId))) { | ||
for (const childId of selectedChildIds) { |
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.
childId
보다는 selectedChildId
로 수정하는 것이 더 연관성이 있어보입니다!! 🤔
for (const childId of selectedChildIds) { | ||
await deleteDoc(childId); | ||
} | ||
history.replaceState(null, null, "/"); |
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.
replaceState
를 활용해서 없어질 예정인 문서 주소를 root 주소로 바꿔주는 모습이 디테일하신 것 같습니다!! 👍👍
await deleteDoc(childId); | ||
} | ||
history.replaceState(null, null, "/"); | ||
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.
삭제될 예정인 주소를 root 주소로 바꿔준 후 root 주소를 history API에 추가하시는 이유가 궁금합니다!! 🤔
const docs = await request("/documents", { | ||
method: "GET", | ||
}); |
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.
fetch 전역 함수에서 GET
메서드의 경우 생략이 가능한걸로 알고있는데 혹시 명확성을 위해 이렇게 하신건가요? 궁금합니다!! 🤔
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.
생략 가능한 줄 몰랐는데 덕분에 하나 더 알고 갑니다!
content: "", | ||
}); | ||
|
||
if (tempDocument.title !== "" || tempDocument.content !== "") { |
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.
@jgjgill 제 생각에는 OR
연산자가 더 나은 것 같습니다!! AND
연산자는 2개 다 비교해야하지만 OR
연산자 같은 경우 한 가지의 경우만 만족하면 되기 때문입니다 😀
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.
채연님 고생 많으셨어요!!!
코드를 보면서 추가로 고민해보시면 좋을 포인트를 짚어본다면
- 함수의 목적을 명확하게하고 하나의 역할만 하게 한다. 목적이 드러나는 네이밍을 한다.
- 책임과 역할을 명확하게 구분한다. (ex. 사용하는 곳에서 책임 져야할까? 아니면 내부에서 책임을 져야할까?)
와 같은 부분이었어요.
코드에 코멘트로 남겨두기도 했는데 혹시 궁금한 것들이 있다면 티타임때 물어봐주세요~~!
고생하셨습니다~~
if (res.ok) { | ||
return await res.json(); | ||
} | ||
|
||
throw new Error("API 연결에 오류가 발생했습니다."); | ||
} catch (e) { |
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.
P3;
res.ok가 아닌 경우에는 어떻게 핸들링 하나요?
4xx번대 에러와 5xx번대 에러의 경우 각기 다르게 핸들링 해야할 것 같은데 여기서는 throw new Error("API 처리 중 이상 발생"); 로 퉁쳐지는 것 같아서요!
ask; 퀴즈
request 함수 관점에서
서버에서 정상적으로 응답이 온 경우!
특히 유저가 실수 해서 난 Error인 4xx번대 에러인 경우 이를 코드상 에러라고 보는 것이 맞을까요?
아니면 정상적으로 동작하는 것으로 보는게 좋을까요?
만약 정상이라고 본다면 어떻게 처리하는 것이 좋을까요?
...options, | ||
headers: { | ||
"Content-Type": "application/json", | ||
"x-username": "colorkite10", | ||
}, |
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.
P1;
이렇게 하게 된다면 headers를 변경하고 싶어도 변경하지 못할 것 같아요.
아래와 같이 headers 또한 수정 가능하게 해두면 어떨까요?
...options, | |
headers: { | |
"Content-Type": "application/json", | |
"x-username": "colorkite10", | |
}, | |
...options, | |
headers: { | |
"Content-Type": "application/json", | |
"x-username": "colorkite10", | |
...options?.headers | |
}, |
const documentEditPage = new DocumentEditPage({ | ||
$target, | ||
initialState: { | ||
docId: "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.
P3;
docId의 ID가 주로 가지는 의미를 생각해보면 "중복되지 않는 유니크한 값" 이라고 생각돼요.
그랬을 때 new라는 값은 이후에 변경될 지언정 적절한지는 살짝 의문이 들었어요.
만약 id 값을 만드는게 막연하다면 uuid 같은 값을 만들어서 부여해보면 어떨까요?
} else if (pathname.indexOf("/documents/") === 0) { | ||
const [, , docId] = pathname.split("/"); | ||
|
||
documentEditPage.setState({ docId }); | ||
|
||
this.setState({ | ||
...this.state, | ||
docId, | ||
}); | ||
} | ||
}; |
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.
p2;
route의 역할을 생각해보면
"url 값에 따라 적절한 화면을 띄워준다." 가 주된 역할일 것 같아요.
그렇다보니 특정 페이지의 state를 바꾼다는 것이 살짝 어색하게 느껴져요.
만약 edit 뿐 아니라 또 다른 페이지들이 생겨난다면..? 이 안에서 if문과 state가 쌓이게 될 것 같은데 그랬을 때 가독성이 나빠질 것 같아요.
여기서는 documentEditPage를 띄우는 것 까지만 하고.
documentEditPage 내에서 init하는 과정에서 docId
를 가져와 init 하는 것이 더 명확한 구분이라고 생각되는데 채연님은 어떠신가요?
} | ||
}; | ||
|
||
const fetchDocuments = async () => { |
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.
P5;
fetch라는 의미를 생각해보면 "무언가를 가져온다" 라는 의미를 가지고 있다고 생각해요. 개발적으론 API 등에서 요청해서 가져온다 라는 의미를 가지고 있죠.
그렇다보니 fetch 함수를 호출한다고 했을 때 response 값이 return 될 것이라 기대가 되는데 여기서는 setState 도 하다보니 기대와 정말 같은 행동일까? 하는 생각이 들었어요.
사용하는 곳에서 await fetchDocuments();
대신 const response = await fetchDocuments()
와 같이 사용하거나 initDocument() 와 같이 행동이 유추 가능한 네이밍을 해보는 것은 어떨까요?
content: "", | ||
}); | ||
|
||
if (tempDocument.title !== "" || tempDocument.content !== "") { |
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.
소소한 TMI
형변환을 통한 검증도 가능할 것 같아요!
if (tempDocument.title !== "" || tempDocument.content !== "") { | |
if (!tempDocument.title || !tempDocument.content) { |
if (confirm("임시저장된 값을 불러올까요?")) { | ||
this.setState({ | ||
...this.state, | ||
doc: tempDocument, | ||
}); | ||
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.
저도 임시 저장 기능을 구현한 것은 매우 좋다고 생각돼요!!! 👍
다만 위에 적은 것 처럼 fetchDocument
라는 함수에서 기대할 수 있는 행동인지는 살짝 의문이 있어요.
if (name === "displayChild") { | ||
const $ul = $li.childNodes[7]; | ||
|
||
if (!$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.
ask;
childNodes array 7번쨰에 ul이 있다는 것을 항상 보장할 수 있을까요?
7번쨰에 ul이 있다는 것을 알아채기 쉽지 않고, 추후 서비스를 만들다 보면 에러가 날 가능성이 꽤 높아보여서요.
initialState: { | ||
text: "add a document", | ||
link: "/documents/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.
오호 버튼에 상태를 넘기는 것이 흥미롭네요~!
} catch { | ||
console.log("페이지가 어디로 갔나봐요. 새로고침을 눌러주세요!"); | ||
} |
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.
P5;
우선 사용자 입장을 생각한 에러 처리 너무 멋지네요~~~! 👍
다만 소소한 우려로 사용자를 위한 에러 문구를 여기서 띄우는 것이 맞을까? 하는 생각이 들었어요.
(추가로 setItem과 페이지가 어디로 갔나봐요 에러는 어떤 상관관계가 있는지 궁금해 졌어요.)
만약 문구를 상황에 따라 다르게 하고 싶다면? console.log가 아닌 다른 방식으로 안내를 하고 싶다면? 등과 같은 상황을 맞이하면 난처 할 것 같거든요.
지금 상황에서는 사용하는 쪽에서 핸들링 할 수 있게 하는 게 좋지 않을까 싶은데 채연님은 어떠신가요?
(이런 경우 사용하는 곳에서 try catch를 하거나 콜백 패턴 등을 사용하기도 해요.)
Co-authored-by: Choi Won Suk <[email protected]>
Co-authored-by: Choi Won Suk <[email protected]>
📌 과제 설명
바닐라 JS를 이용하여 Notion 페이지를 클로닝하였습니다.
npx server -s
를 입력하시면 실행 가능합니다.👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
목적
912686c
: 강의에서 배운 대로 LinkButton을 활용하였었으나, 루트 문서를 생성하는 것과 하위 문서를 생성하는 역할을 통일하고 싶어서 건드렸다가 fix의 연속을 겪었습니다. 코드를 작성할 때 구조를 먼저 생각하는 것에 중요성을 다시금 깨닫게 된 계기가 되었읍니다... state를 관리해주는 것은 정말 어려운 것 같습니다. 😢😢😢😢
수정할 사항
추가할 기능
소감
과제 하면서 재미도 있었지만 아쉬움이 많이 남습니다. 좀 이해가 간다 싶을 때에 마감기간이 되어 버려서...
부족한 코드이지만 이해하고 적용하고자 노력해봤습니다!