-
Notifications
You must be signed in to change notification settings - Fork 25
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
[Project1/백준원] Notion 클로닝 프로젝트 #10
The head ref may contain hidden characters: "5/#5_\uBC31\uC900\uC6D0_working"
Conversation
* SidebarContainer 내부에 DocumentList 렌더링 * DUMMY_DATA로 테스트 * api 호출 성공
* router.js, storage.js 추가 * SidebarContainer의 DUMMY_DATA 삭제
* <ul> 수정 * Document 클릭 시 페이지로 이동
* 변수명 지정하고 수정
* reset.css 설정 * 아이콘 설정 * 컴포넌트 구조 설정
* request(url, options) => fetchDocuments(documentId, options)
* DocumentList 항목 열기/닫기 * + 버튼 클릭 시 하위 문서 추가 * 삭제 버튼 클릭시 해당 문서 삭제
* DocumentList 맨 아래 버튼 추가 * SideBar 맨 아래 버튼 추가
* 편집 중인 문서의 하위 문서 링크 렌더링
* Sidebar / DocumentEditPage 디렉토리 생성
* 문서 클릭하면 해당 문서 제목으로 title 이름도 변하도록 구현
* sidebar 맨 위에 현재 workspace를 설명해주는 header 문구 추가
* CSS 수정 * SideBar 클릭 시 하이라이트 안되는 에러 수정 * generateTextIndent : 들여쓰기 * grenerateTitle : title 값이 없으면 UNTITLED 값이 나오도록
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.
항상 준원님이 작성하신 코드볼때마다 저와 정반대로 기본기?.. 코드 작성전의 준비성?... 같은 부분이 철저하신것같은 생각이 들어요!
특히 이번 과제에서도 constants.js
이나 component 파일에서 별도로 상수 선언하신 부분에서 놀랐습니다!
이번에도 많이 배워갑니다 😀 졸작도 하셔서 바쁘실텐데 I am 화이팅 에요
if (removedDocumentId === FIRST_DOCUMENT_ID) { | ||
alert("첫 페이지는 지우지 말아주세요 :D"); | ||
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.
(ASK) 첫 페이지를 삭제하지 않는 이유가 있을까요오?
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 삭제를 하면 첫 페이지로 이동하도록 구현했습니다!
import { setDocumentTitle } from "./utils/validation.js"; | ||
|
||
export default function App({ $target }) { | ||
let timer = 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.
( P4 ) timer 라는 변수는 아래 디바운스 기능을 구현할때 사용되는 변수인데 최상단에 선언되어있어 의도를 알기 까지 고민이 필요할것같다라는 생각이 들었습니다! 🤔 상단에 변수를 선언하는것도 좋지만 사용되어지는 곳 위에서 선언하는것이 좀 더 의미를 알기 좋지 않을까 라는 의견 드려봅니다!
`; | ||
}; | ||
|
||
const renderDocuments = (nextDocuments, depth) => ` |
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 ) 저도 이번 과제에서 리스트를 구현하며 고민 했던 부분이고 멘토님께 여쭤봤던 부분이라 한번 의견 드려봅니다!
다름 아닌 시맨틱 태그 관련 인데요!
제가 잘 이해했다면 현재는 List 들을 담는 태그를 div
태그를 사용하셨고 그 안의 요소를 ul
태그를 통해 만드신것같은데요!
List 들을 담고 있는 List 박스의 태그가 List를 담고 있기 때문에 ul
태그를 사용하고, 그안의 하나의 Document 가 li
태그로 이루어졌다면 어땟을까요 🤔
그리고 각각의 li
태그 안에는 자식을 담을 u
l 태그가 존재하고 그 안에서 또 해당 자식 Document 들을 li
태그로 정의 해보신다면 의미론적으로 올바르게 사용되지 않을까 의견드려봅니다
</ul> | ||
` | ||
) | ||
.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.
( P3 ) innerHTML 로 훌륭하게 만들어 주신것같아요!
그치만 개인적으로는 안에서 사용 되어지는 함수도 많고 해당 부분을 따로 Component로 구현하신다면 renderButton
등 여러 함수도 함께 component 내부에 정의하시는 방법을 통해 DocumentList.js
파일의 가독성을 높일 수 있지 않을까 라는 생각을 해봤습니다 😉 ( 특별한 의도가 있으시다면 제가 발견못한 부분인것 같아서 알려주시면 감사드리겠습니다 🙏 )
$documentList.innerHTML = ` | ||
${documents.length > 0 ? renderDocuments(documents, 1) : ""} | ||
`; |
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 ) 위 component 분리에 대한 의견의 연장선으로
만약 List.js 라는 파일에 List 를 구현하는 component를 만드신다면!
해당 부분도
this.state.forEach((list) => {
new List({
state : list,
target : $documents
})
})
로 변경을 통해 간단하게, 그리고 가독성도 높게 만들 수 있지 않을까... 라는 생각을 해봤습니다 🫠 적고 나니까 간단한지에 대한 의견은 의문이 드네요 ㅎㅎ..
@@ -0,0 +1,27 @@ | |||
import { ADD, NEW } from "../../utils/constants.js"; | |||
|
|||
export default function DocumentAddButton({ $target, initialState, onAdd }) { |
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 ) 이번 과제에서 Document
를 추가할 수 있는 부분이
각각의 리스트들과 Root
문서를 생성할때 등 여러곳에서 생성이 가능한것같습니다!
그렇다보니 해당 Component
는 Root 문서를 추가할때 사용되는 component인것 같은데 DcoumentAddButton
은 어느 부분에 추가할 수 있는 Component
인지 알기 쉽지 않을것같아서 예로 RootDocumentAddButton
같은 이름이 어떨까 의견드려봅니다 😀
$button.addEventListener("click", () => { | ||
onAdd(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 ) 이또한 저도 저번에 리뷰 받은 부분인데 해당 addButton
은 DocumentList.js Component
의 state
가 변경될때 마다 DocumentList 내의 this.render
를 통해 계속해서 다시 만들어 지는것 같습니다!
그렇게 계속 만들어질때마다 계속해서 이벤트가 새롭게 생성되어질 것 같은데 이부분도 DcoumentList.js
내부에서
각각의 버튼들을 이벤트 위임을 통해 핸들링 하신부분과 같이 DocumentList.js 내부
에서 해당 버튼에 대한 이벤트를
핸들링 해보시면 어떨가 라는 라는 의견 드려봅니다
<button title="삭제" class="${DELETE}" type="button"> | ||
<i title="삭제" class="fa-regular fa-trash-can ${DELETE}"></i> | ||
</button> |
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 ) ㅋㅋㅋㅋㅋ 분명 버튼이 있는데 어딨지... 하고 찾았는데 귀렵게 숨겨져 있었네요 🤣
좀 더 존재를 알수있게 제목 옆쪽이나 중심에 가깝게 배치해보시면 어떨까요!
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;
이번 프로젝트에서 버튼을 추가하는 곳이 많은데 버튼을 만드는 함수를 따로 js 파일로 생성하셔서 관리하셔도 좋을 것 같습니다!👍
<div class="titles"> | ||
${documents | ||
.map(({ id, title }) => `<p data-id="${id}" class="title">${title.length === 0 ? UNTITLED : title}</p>`) | ||
.join("")} | ||
</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.
( P3 ) 의미론적 태그에 대한 의견 드립니다!
아래 Document List.js
에서도 비슷한 의견 남겨드렸는데 현재 사용된 div
와 p
태그를 ul
와 li
로 바꾸시면 의미론적으로 좋은 태그가 될것 같습니다!
} | ||
}; | ||
|
||
export const generateTextIndent = (depth) => 12 * depth; // 들여쓰기 |
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 ) 이 의견은 정말 개인적인 의견이긴 하지만..
코드를 읽다보니 해당 부분에 대해서 저와 동일하게 사용하신 부분이라 한번에 알 수 있었지만,
해당 부분은 오히려 함수 사용으로 의도를 파악하기까지 좀 더 시간이 걸릴 수 있는 부분이 아닐까 생각이 들어서 의견드려봅니다
style="padding-left: ${generateTextIndent(depth)}px"
요걸
style="padding-left: ${12 * depth }px"
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.
준원님 과제하신거 잘 보구 갑니다!
졸프하시느라 시간도 부족하실텐데 너무 고생많으셨습니다 ㅎㅎ
이제 곧 졸프도 끝나시고, 좀 더 여유있어진 준원님 일상을 저도 기대해봅니다🫡
<button title="삭제" class="${DELETE}" type="button"> | ||
<i title="삭제" class="fa-regular fa-trash-can ${DELETE}"></i> | ||
</button> |
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;
이번 프로젝트에서 버튼을 추가하는 곳이 많은데 버튼을 만드는 함수를 따로 js 파일로 생성하셔서 관리하셔도 좋을 것 같습니다!👍
export const DOCUMENTS_ROUTE = "/documents"; | ||
export const NEW = "new"; | ||
export const UNTITLED = "제목 없음"; | ||
export const NEW_PARENT = "NEW-PARENT"; | ||
export const ADD = "add"; | ||
export const DELETE = "delete"; | ||
export const OPENED_ITEMS = "opened-items"; | ||
export const FIRST_DOCUMENT_ID = 102128; |
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.
준원님 Notion 프로젝트 하신다고 정말 고생많으셨습니다!!
학교 생활 병행하면서 하신다는게 저한테는 정말 리스펙한거 같아요ㅜㅜ 항상 리뷰할때 코드 보면 굉장히 탄탄한? 느낌을 자주 받고 가는 것 같아요!! 그리고 컴포넌트 설계와 분리도 매우 깔끔하게 잘 하신 것 같아서 많이 지식을 얻는 것 같습니다!!
this.route = () => { | ||
const { pathname } = window.location; | ||
if (pathname === "/") { | ||
setDocumentTitle("Notion"); | ||
return; | ||
} | ||
if (pathname.indexOf(DOCUMENTS_ROUTE) !== 0) return; | ||
|
||
const [, , documentId] = pathname.split("/"); | ||
documentEditPage.setState({ | ||
documentId: isNaN(documentId) ? documentId : parseInt(documentId), | ||
}); | ||
|
||
if (!isNaN(documentId)) { | ||
sidebarContainer.setState({ | ||
selectedId: parseInt(documentId), | ||
}); | ||
} | ||
}; |
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.
P4;
if (pathname.indexOf(DOCUMENTS_ROUTE) !== 0) return;
이 코드에서 /document가 포함된 url이 아니면 종료하는 것 보다는 if문으로 오히려 /document가 포함된 url에 대한 로직을 수행하고
if문 안에서 return을 하거나 else 키워드를 통해 예외 url에 대한 처리를 하는 것이 더 예외 url에 대처하기 쉽고 가독성에 좋을 것 같아요!!
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.render = () => { | ||
if (!this.state.document || !this.state.document.documents) 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.
P4;
이 if문에서 옵셔널 체이닝을 이용하면 더 깔끔하게 처리할 수 있을거 같아요!
if(!this.state?.document?.documents) return;
옵셔녈 체이닝을 이용하서 객체 속성 값을 하나씩 하나씩 확인해서 중첩된 객체의 false 값을 찾을 수 있습니다!
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.
옵셔널 체이닝 생각을 못했네요... 좋은 리뷰 감사합니다
$header.addEventListener("click", (e) => { | ||
const { target } = e; | ||
|
||
if (target.classList.contains("delete")) { | ||
onDelete(this.state.documentId, this.state.documentId); | ||
} | ||
}); |
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;
onDelete함수의 2개의 인자가 같은 값을 넘겨주고 있는 것 같아요! this.state.documentId 값을 1개만 전달하고 활용하면 더 좋을 것 같습니다!
export const generateTitle = (title) => { | ||
if (typeof title === "string") { | ||
return title.length > 0 ? title : UNTITLED; | ||
} | ||
}; | ||
|
||
export const setDocumentTitle = (title) => { | ||
if (typeof title === "string") { | ||
document.title = generateTitle(title); | ||
} | ||
}; |
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.
P4;
제가 이해를 잘 했는지 모르겟는데 만약 두 개의 함수가 각각 사용이 따로 되지 않는다면 setDocumentTitle안에서 모두 처리하셔도? 좋을 것 같아요!!
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 [, , documentId] = pathname.split("/"); | ||
documentEditPage.setState({ | ||
documentId: isNaN(documentId) ? documentId : parseInt(documentId), |
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;
혹시 코드의 의도가, documentId가 숫자가 아니라면 숫자로 바꾸어서 주세요~ 가 맞다면
documentEditPage.setState({
documentId: isNaN(documentId) ? parseInt(documentId) : documentId,
});
처럼 반대로 되어야 할 것 같아요 👀
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.
반대로 코드를 작성했네요... 얼른 수정하겠습니다!
export const ADD = "add"; | ||
export const DELETE = "delete"; | ||
export const OPENED_ITEMS = "opened-items"; | ||
export const FIRST_DOCUMENT_ID = 102128; |
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, | ||
selectedId: this.state.selectedId, | ||
}); | ||
documentAddButton.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.
p5;
documentAddButton이 DocumentList.js 에서도 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.
준원님 과제하시느라 고생 많으셨습니다!
코드도 깔끔하고 읽기 좋게 잘 작성해주셨는데 배포까지 하신 것 들어가보고 놀랐네요! css까지 완벽하게 작성해주신것 같습니다!!
컴포넌트화, 모듈화를 했는데 조금 더 의존성을 낮출 수 있는 방법이 있는지 또는 제 코드에 불필요한 부분이 있는지 궁금합니다
전체적으로 잘 작성해 주셨는데 옵저버 패턴이나 커스텀 이벤트를 통해서도 의존성을 낮추는 작업을 해보실 수 있을 것 같아요
변수명, 함수명 그리고 디렉토리 구조에 대해 피드백 받고 싶습니다.
* onXXX 형태의 콜백 함수 명도 괜찮았고, 전체적으로 깔끔하게 작성해주신것 같아서 너무 좋았습니다!
* 의미적으로 크게 문제는 없지만 한번 고민해볼 것은 ooooPage 라는 이름이 현재 사실 Editor 역할을 하는 컴포넌트에게 Page를 붙여주어서 헷갈릴 수도 있지 않을까 싶어요.
* Page라는 이름은 주로 route에 진입했을 때 Page 최상단 컴포넌트를 의미하는 경우가 많은데, Page가 어떤 컴포넌트에 붙으면 좋을지?만 고민해보셔도 좋을것 같다는 생각이들었어요!
여러 컴포넌트에서 사용되는 상수명을 SNAKE 방식으로 선언하여 구현하였는데 이 부분에 대해서 피드백 받고 싶습니다.
* 상수로 나눠주신부분 좋았어요! 지금도 괜찮지만 조금 더 수정을 해본다면 constant도 다 같은 타입의 constant가 아닐거에요. 예를들어 route path를 위한 constant가 있을 거고. 렌더링 text를 위한 display text 상수도 있겠죠?! 요런 것들이 현재 Constant 한 파일에 모여있는데 요것도 분리해서 관리하거나. 여러곳에서 사용하는 상수가 아니라면 실제 사용하는 파일 최상단에 선언해서 관리할 수 도 있을 것 같아요.
onDelete 를 구현할 때 삭제를 한 후 첫 페이지로 이동하도록 구현하였는데 이 부분이 어색하지는 않은지 궁금합니다.
* 아하! 첫페이지 이동을 위해 첫페이지 삭제를 막아두신것이군요!
* 첫페이지가 일반적인 페이지들의 Home 이라고 봤을 때는 어색하지 않은 것 같아요.
* 하지만 현재 구현체에서 첫페이지는 별도의 Home 컴포넌트가 아닌 다른 Document와 같은 Document 타입이기 때문에 첫페이지에 삭제버튼은 있지만 지울수는 없다던가 살짝 어색한 부분이 있는 것 같기는 합니다!
Editor에 MarkDown 형식을 구현하고 싶은데 조금 쉽게..?? 간단하게..? 구현할 수 있는 방법이 있을까요??
- 실제 raw한 값과 display 값이 다르게 보여주는 방법으로 구현이 가능할 것 같은데요.
보통 크게 2가지 방식으로 나뉘는 것 같아요.
* input focus, blur 를 활용해서 focus 상태에 따라 input 컴포넌트와 display 값을 보여주는 컴포넌트를 번갈아 보여준다.
* input 같아 보이는 fake input을 만들어 display를 보여주고 실제로 hidden 된 찐 input에 raw 한 값을 처리하는 경우
* 이 방법을 통해 마크다운 파싱하는 함수를 만들고 위와 같은 특정시점에 trigger가 있을 때 입력값을 파서 함수에 넣어 결과값을 Display 하는 방법으로 구현이 가능할 것 같아요
과제하시느라 고생 많으셨습니다!!
if (pathname.indexOf(DOCUMENTS_ROUTE) !== 0) return; | ||
|
||
const [, , documentId] = pathname.split("/"); | ||
documentEditPage.setState({ | ||
documentId: isNaN(documentId) ? documentId : parseInt(documentId), | ||
}); | ||
|
||
if (!isNaN(documentId)) { | ||
sidebarContainer.setState({ | ||
selectedId: parseInt(documentId), | ||
}); | ||
} | ||
}; |
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.
현재는 1가지 Path 형태밖에 없지만 나중에 Route path가 복잡해지면 이쪽 코드도 복잡해지고 에러가 발생할 수 있을 것 같아요!
switch문이나 정규식을 사용해보면 어떨까요?
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.
제가 아직 정규식은 익숙하지 않아서 switch문을 사용해보겠습니다..!
|
||
export const generateTitle = (title) => { | ||
if (typeof title === "string") { | ||
return title.length > 0 ? title : UNTITLED; | ||
} | ||
}; | ||
|
||
export const setDocumentTitle = (title) => { | ||
if (typeof title === "string") { | ||
document.title = generateTitle(title); | ||
} | ||
}; |
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.
저도 이 부분 로직이 비슷한 것 같아서 합쳐서 하나로 사용할 수 있지 않을까 싶네요!
export const getItem = (key, defaultValue) => { | ||
try { | ||
const storedValue = storage.getItem(key); | ||
return storedValue ? JSON.parse(storedValue) : defaultValue; | ||
} catch (e) { | ||
return defaultValue; | ||
} | ||
}; | ||
|
||
export const setItem = (key, value) => { | ||
storage.setItem(key, JSON.stringify(value)); | ||
}; | ||
|
||
export const removeItem = (key) => { | ||
storage.removeItem(key); | ||
}; |
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.
P4;
이 부분은 싱글톤 패턴으로 인스턴스화 해서 사용해 볼 수 있을 것 같아요
if (res.ok) { | ||
return await res.json(); | ||
} | ||
throw new Error("API 요청 실패"); |
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.
Request 함수 잘 말아주셨는데요~ res.ok가 아닌 경우는 어떻게 동작할까요?
400~500 번대 에러의 경우에는 다르게 동작해야할 것 같은데 지금 API 처리 중 뭔가 이상합니다. 로만 빠지도록 되어져 있는 것 같아요!
API response에 여러 상태가 있을 것 같은데 각각 에러는 어떤 의미가 있고 각각 어떻게 처리해주면 좋을까요?!
고민해보시면 좋을 만한 내용 전달드려욧!
import { setDocumentTitle } from "./utils/validation.js"; | ||
|
||
export default function App({ $target }) { | ||
let timer = 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.
그런 의미에서 debounce를 재사용 가능한 별도의 util 형태로 설계해서 작성해보시면 어떨까요?!
참고
유틸을 만들때 기존에 비슷하게 구현된 것 들은 어떻게 구현했는지? 어떤 props를 주고 받는지를 보시고 이해하시면 코어한 유틸을 만들때 조금 더 도움 되실 것 같아요~
if (removedDocumentId === FIRST_DOCUMENT_ID) { | ||
alert("첫 페이지는 지우지 말아주세요 :D"); | ||
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.
오 저도 궁금해요!
const createdDocument = await fetchDocuments("", { | ||
method: "POST", | ||
body: JSON.stringify({ | ||
title: "", | ||
parent: getItem(NEW_PARENT, 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.
요런 정규화된 코드는 별도의 함수로 빼면 사용하는 곳에서 깔끔할 것 같다는 생각이 드네요!
새로운 게시글 만든느 버튼이 전혀 다른곳에 생긴다 하더라도 해당 핸들러에 게시글 쓰는 Api가 뭐였지? 무슨 body를 넣어줘야하지? 이런 고민이 없게 만들어두어도 좋을 것 같아요.
const createdDocument = await fetchDocuments("", { | |
method: "POST", | |
body: JSON.stringify({ | |
title: "", | |
parent: getItem(NEW_PARENT, null), | |
}), | |
}); | |
const createdDocument = await postNewDocument(); |
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.
별도로 함수로 빼서 수정해보겠습니다!
await fetchDocuments(removedDocumentId, { | ||
method: "DELETE", | ||
}); |
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 { target } = e; | ||
|
||
if (target.classList.contains("delete")) { | ||
onDelete(this.state.documentId, this.state.documentId); |
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.
onDelete가 비동기 함수인데 호출부에서 await이 없네요.
아래와 같은 상황이 있다고 가정해 봤을 때 비동기함수는 비동기로 동작할 수 있도록 해주시는게 좋을 것 같아요
게시글 삭제 후에 document list 새로고침 하는 로직이 있다고 할 때 onDelete 함수를 호출해 delete API를 쏘았지만 서버 문제로 10초가 걸리고 document list를 조회하는 API는 1초가 걸린다고 하면 onDelete API의 성공 응답을 받기 전에 document list를 조회하게 되고 그러면 10초뒤에 실제 게시글은 삭제되지만 document list에는 그대로 남아있는 문제가 발생할 것 같아요.
<input type="text" name="title" class="title" placeholder="${UNTITLED}" autofocus/> | ||
<textarea name="content" class="content" placeholder="내용을 입력하세요."></textarea> |
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.
title Placeholder를 상수를 사용해서 잘 분리해주셨네요!
요렇게 display text를 별도의 변수에 사용하는 경우가 다국어 처리할 때 사용하게 되는데, i18n 관련해서 찾아보시면서 공부하셔도 좋을 것 같네용!
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.
네! i18n 관련해서 찾아보면서 공부해보겠습니다. 좋은 리뷰 감사합니다
📌 과제 설명
VanillaJS로 노션을 클로닝했습니다.
배포 : https://joonwon-notion.vercel.app/
👩💻 요구 사항과 구현 내용
+
버튼이 있습니다. 해당 버튼을 클릭하면, 클릭한 Document의 하위 Document로 새 Document를 생성하고 편집화면으로 넘깁니다.(...)
처리를 합니다✅ PR 포인트 & 궁금한 점