-
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 #7
base: 3/#4_byunguno
Are you sure you want to change the base?
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.
안녕하세요 건오님!
첫번째 개인 프로젝트 고생 많으셨습니다.
요구 사항을 최대한 깔끔하게 구현하신것 같습니다😆
프로젝트 기간동안 건오님과 의견을 주고받은 덕분에
어려운 문제를 빠르게 해결할 수 있었습니다!
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/Editor.js
Outdated
$editor.innerHTML = ` | ||
<input type="text" name="title" style="width:600px" value="${this.state.title}" /> | ||
<textarea name="content" style="width:600px;height:400px;border:1px solid black; padding:8px;">${this.state.content}</textarea> | ||
`; | ||
|
||
this.render = () => { | ||
$editor.querySelector("[name=title]").value = this.state.title; | ||
$editor.querySelector("[name=title]").placeholder = "제목을 입력해주세요"; | ||
$editor.querySelector("[name=content]").value = this.state.content; | ||
$editor.querySelector("[name=content]").placeholder = "내용을 입력해주세요"; | ||
}; |
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.
이건 뭔가 신선한 방법이네요👍. setState
후에 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.
건오님 프로젝트 진행 하시느라 고생하셨습니다.
리뷰 진행하면서 저도 많이 배우고 갑니다!
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에서 list의 열림, 닫힘 상태를 LocalStorage에서 관리하고 있는데, 더 좋은 방법이 있을까요?
ex) li 태그안에 isToggle같은 커스텀 상태를 만들어서 가져오고 변화를 줄 수 있는 방법이 있나요?
토글 값을 저장하고 있어야 한다면 localStorage도 마냥 나쁜 전략은 아닌 것 같습니다. 말씀하신 li 태그에 커스텀 상태를 만드는 접근법이 data 속성을 사용하는 방법입니다.
이번 과제를 bottom - up 방식으로 하다보니 최상단 App에서 상태관리를 하지 못하여서 top-down 방식으로 다시 노션클로닝을 하려하는데 top-down을 하면서 어떤 것을 기준으로 컴포넌트를 나누는게 좋을까요?
top down의 경우 추상화가 중요합니다. 먼저 큰 영역을 기준으로 분리를 해보세요. (헤더, 푸터, 사이드바, 메인, ...) 그 다음 헤더에는 버튼과 아이콘 등이 들어가야 하고, 메인을 구현할 때도 버튼과 아이콘이 들어가야 한다면 각 영역에 중복되는 요소들이 등장하게 되는데 이 경우 중복되는 요소를 컴포넌트로 뽑아내는 방식으로 잔행하면 됩니다.
현업에서는 에러 처리를 어떻게 하나요? ex) 과제를 하면서 if문을 너무 많이 쓴 것 같은데 현업에서도 if문을 쓰나요? 아니면 다른 조건문으로 에러 처리를 하나요?
조건문이라고 해도 if 아니면 switch죠. 조건문 자체가 많이 등장하는 것은 문제가 아닙니다. 문제라는 생각이 든다면(짧은 구간 내 너무 많은 조건문이 등장한다면) 조건 식을 재검증해본다거나, 함수로 분리하는 방식으로 줄여야 합니다.
예외 처리를 조건에 부합하지 않는 경우 Error(Exception)을 발생시키고 try..catch로 처리를 하거나, 반환형을 {data: ..., result: "fail"} 같은 형태로 규격화해서 처리하기도 하고 다양한 방식으로 처리할 수 있습니다.
Document List들을 sidebar에서 보여줄 때, document의 자식이 있을 때, 재귀함수를 써서 보여주는 방식을 사용했는데 다른 방법이 더 있을까요?
재귀함수면 단순히 반복문으로 대체할 수 있을테고, API에서 제공하는 데이터 형태에 따라 이벤트가 발생했을 때 해당하는 데이터를 찾아서 즉석으로 엘리먼트를 만들어주는 방식도 가능할 것 같습니다.
#close .toggleBtn { | ||
transform: rotate(0deg) | ||
} | ||
|
||
#open .toggleBtn { | ||
transform: rotate(90deg); | ||
} |
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.
- id selector를 스타일에 활용하는 건 최대한 지양하는 편이 좋습니다. (재활용 불가)
- 그리고 id를 네이밍할 때는 가능하면 2개 이상의 단어를 조합해서 엘리먼트의 정체가 무엇인지 명확히 표현해주는 편이 좋을 것 같습니다. (e.g. modal-open, toggle-close, ...)
src/components/EditPage.js
Outdated
|
||
export default function EditPage({ $target, initialState, getTitleChange }) { | ||
const $editPage = document.createElement("div"); | ||
$editPage.className = "Edit-page"; |
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.js
Outdated
|
||
this.state = initialState; | ||
|
||
let postLocalSaveKey = `temp-post-${this.state.documents.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로 선언해도 되지 않을까요?
src/components/Sidebar.js
Outdated
|
||
const $HomeButton = document.createElement("button"); | ||
$HomeButton.className = "home-button"; | ||
$HomeButton.innerHTML = "Notion 홈으로"; |
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.
HTML을 입력하는 게 아니라면 textContent를 사용하셔도 됩니다.
const documents = { | ||
title: `제목 없음`, | ||
parent: id, | ||
}; | ||
const childDoc = await request(`/`, { | ||
method: "POST", | ||
body: JSON.stringify(documents), |
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.
단일 문서니까 documents보다는 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.
documents로 데이터를 주고받다 보니 documents로 데이터를 넘겨야겠다고 생각했나봅니다..ㅎㅎ😅
src/components/NewDocBtn.js
Outdated
export default function NewPostBtn({ $target, addPost }) { | ||
const $newButton = document.createElement("div"); | ||
$newButton.className = "notion-newpage-button"; | ||
$newButton.role = "button"; | ||
|
||
$target.appendChild($newButton); | ||
|
||
this.render = () => { | ||
$newButton.innerHTML = ` | ||
<div class="notion-focusable" role="button"> | ||
<div class="new-wrapper"> | ||
<div class="new-plus-button">+</div> | ||
<div>새 페이지</div> | ||
</div> | ||
</div> |
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.
NewPostButton도 좋지만, parameter를 통해서 text와 icon 등을 입력 받는다면 버튼 컴포넌트의 재사용성을 더 높일 수 있을 것 같습니다. (지금 당장 사용 범위를 확대할만한 부분이 없다면 일단은 이대로도 괜찮을 것 같아요.)
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.js
Outdated
@@ -0,0 +1,85 @@ | |||
import PostList from "./DocList.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.
Nit: 특별한 일이 없다면(이름 충돌 문제 등) default import을 하는 경우엔 파일 이름이랑 동일하게 맞춰주시면 좋을 것 같습니다.
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 부분을 신경 못 썼네요ㅠㅠ
📌 과제 설명
👩💻 요구 사항과 구현 내용
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점