-
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 과제 #35
base: 4/#5_MinwooP
Are you sure you want to change the base?
Conversation
eslint-config-prettier로 설정
- App.js, main.js 구현 - SideBar, EditPage 컴포넌트 생성 - 기본 css 설정 - util 함수 정의
- 더미데이터로 테스트
- 조회 후 좌측 SideBar에 렌더링
- spreadButton 클릭 시, 하위 document 펼치기 - title 클릭 시, 현재 선택된 document 변경 - + 버튼 클릭 시, 현재 document 내 하위 document 추가 - - 버튼 클릭 시, 현재 document 삭제
- 각 document 클릭, 생성, 삭제 시 url 변경 - url 변경 시, app의 selectedDocumentId 변경 후 route() 함수 호출을 통해 리렌더링 - 새로고침 시에도 버벅거림 없이 동작하도록, app의 initialState를 null이 아닌 현재 url의 pathname에 따라 설정
- 특정 document 삭제 시, document가 spreadList에 있다면 삭제 - 하위 document 생성 시, 현재 parent의 childDocumenList가 닫혀 있다면, 펼치기 - 메인 페이지에서는 editPage의 title이나 content 수정 못하도록 방지 - sideBar header의 icon, title 클릭 시, 메인 페이지로 이동 - 각 버튼 hover시, 색상 변경 - document의 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.
리뷰 완료입니다. 고생하셨습니다~
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<meta charset="UTF-8" /> |
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.
meta 설정에 있어 어떤 값들이 들어갈 수 있는지 이참에 찾아보면 좋을것 같네요~
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.
넵 ! meta 태그를 이용한 검색 엔진 최적화를 위해 노력하겠습니다 ㅎㅎㅎ
@@ -0,0 +1,5 @@ | |||
{ |
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.
Live 서버로 앱을 실행해봤을때 처음 실행하면 api 404에러가 노출되는것 같은데요!
혹시 앱을 띄우는 별도 방법이 있다면 여기에 해당 라이브러리와 스크립트를 명시해주세요
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.
package.json 파일의 scripts 항목에 다양한 명령어를 설정해두고 콘솔에서 실행할 수 있다는 사실을 처음 알았네요,,! 또 하나 배웁니다 ㅎㅎㅎ
initialState: [], | ||
onAddRootDocument: async () => { | ||
const createdRootDocument = await createDocumentAPI(); | ||
history.pushState(null, null, `/${createdRootDocument.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.
요런 pushState같은 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.
pushState, replaceState 같은 간단한 api라도 그 동작을 더 명확하게 나타내는 방향으로 추상화한다면 더 직관적이고 가독성이 좋아질 것 같습니다 ! 반영했습니다 👍🏻
if (className === "spreadButton") { | ||
toggleSpreadIcon(e.target); | ||
toggleDisplay(documentItem.querySelector(".childDocumentList")); | ||
toggleToSpreadDoucmentList(id.toString()); | ||
} else if (className === "addChildDocumentButton") { | ||
onAddChildDocument(id); | ||
} else if (className === "deleteDocumentButton") { |
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.
바...반영하셨나요?
if (this.state.selectedDocumentId) { | ||
setTempDocumentToStorage(this.state.selectedDocumentId, { | ||
title: e.target.value, | ||
content: contentElement.value, | ||
tempSaveDate: new Date(), | ||
}); | ||
|
||
if (timerForTitle != null) { | ||
clearTimeout(timerForTitle); | ||
} | ||
timerForTitle = setTimeout(async () => { | ||
await modifyDocumentAPI( | ||
this.state.selectedDocumentId, | ||
e.target.value, | ||
contentElement.value | ||
); | ||
removeTempDocumentFromStorage(this.state.selectedDocumentId); | ||
updateSideBar(); | ||
}, 2000); |
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.
여기서 사용하려는 기법의 형태가 디바운스라고 부르는 유틸 함수일텐데요
자주 사용하는 함수이니 별도의 함수로 분리해서 하고자 하는 함수를 넘기는 형태로 사용해보세요
이렇게 구현을 하다보면 디바운스나 쓰로틀을 여러 파일에서 사용하고 싶을 때 매번 구현을 해야할거에요
2000같은 값들도 매직넘버이니 명확하게 상수 이름을 지어주시구요!
추가로 쓰로틀과의 차이점은 뭐가 있는지도 공부해보시면 좋을것 같습니다.
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.
디바운스 함수 유틸화 및 매직넘버 상수화 했습니다 ! 추가로 쓰로틀도 공부해봐야겠습니다 🥺
spreadDocumentList && | ||
spreadDocumentList.includes(document.id.toString()) |
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.
자주 쓰이는 로직은 유틸성 함수로 만들어봐야겠습니다 👍🏻😊
return newElement; | ||
}; | ||
|
||
export const createDocumentElement = (document, spreadDocumentList) => { |
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.
이 함수에서 엄청 많은 일을 하고 있는데 쪼개서 사용하는 형태로 진행해보시져
@@ -0,0 +1,8 @@ | |||
export const MAIN_PAGE_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.
+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.
오 전역적으로 활용되는 String 리터럴이나 그걸 만드는 함수를 이렇게 export 해서 쓰시는군요..! 대박입니다
|
||
export const removeFromSpreadDoucmentList = (documentId) => { | ||
const spreadDocumentList = getSpreadDocumentFromStorage(); | ||
if (spreadDocumentList && spreadDocumentList.includes(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.
요런 형태의 코드들이 많은데 nullish 병합 연산자 한번 검색해서 사용할 수 있는 케이스들을 살펴보시는건 어떤가여
if (spreadDocumentList && spreadDocumentList.includes(documentId)) { | ||
const newList = spreadDocumentList.filter( | ||
(spreadId) => spreadId !== documentId | ||
); | ||
setSpreadDocumentToStorage(newList); | ||
} else { | ||
if (spreadDocumentList) { | ||
setSpreadDocumentToStorage([...spreadDocumentList, documentId]); | ||
} else { | ||
setSpreadDocumentToStorage([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.
리스트를 만들어주고 마지막에 할당을 다같이 하는 방향으로 가면 코드가 깔끔해질수도 있을것 같아여
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 상수화 등 배울게 많은 코드인것 같아요
overrides: [ | ||
{ | ||
env: { | ||
node: true, | ||
}, | ||
files: [".eslintrc.{js,cjs}"], | ||
parserOptions: { | ||
sourceType: "script", |
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 (className === "spreadButton") { | ||
toggleSpreadIcon(e.target); | ||
toggleDisplay(documentItem.querySelector(".childDocumentList")); | ||
toggleToSpreadDoucmentList(id.toString()); | ||
} else if (className === "addChildDocumentButton") { | ||
onAddChildDocument(id); | ||
} else if (className === "deleteDocumentButton") { | ||
onDeleteDocument(id); | ||
} else { | ||
onChangeSelectedDocumentId(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.
이벤트 버블링 캡쳐링 스터디 때 그 부분이군요~ 이거는 근데 진짜 도움이 많이 될것같아요 감사합니다.
if (this.state.selectedDocumentId) { | ||
setTempDocumentToStorage(this.state.selectedDocumentId, { | ||
title: e.target.value, | ||
content: contentElement.value, | ||
tempSaveDate: new Date(), | ||
}); | ||
|
||
if (timerForTitle != null) { | ||
clearTimeout(timerForTitle); | ||
} | ||
timerForTitle = setTimeout(async () => { | ||
await modifyDocumentAPI( | ||
this.state.selectedDocumentId, | ||
e.target.value, | ||
contentElement.value | ||
); | ||
removeTempDocumentFromStorage(this.state.selectedDocumentId); | ||
updateSideBar(); | ||
}, 2000); |
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.
역시 이 부분은 다 똑같네요 ㅎㅎ
tempSaveDate: new Date(), | ||
}); | ||
|
||
if (timerForContent != 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.
!==
null | ||
); | ||
|
||
if (tempDocument && tempDocument.tempSaveDate > updatedAt) { |
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.
맞는 포인트인지는 잘 모르겠지만 저는 다음부터는 조건절은 웬만하면 다 상수화 시키려고 합니다. ㅎㅎ
spreadDocumentList && | ||
spreadDocumentList.includes(document.id.toString()) |
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 { | ||
--color-white: #ffffff; | ||
--color-light-white: #fafafa; | ||
--color-little-white: #cccccc; | ||
--color-dark-white: #bdbdbd; | ||
--color-dark-grey: #4d4d4d; | ||
--color-grey: #616161; | ||
--color-light-grey: #7c7979; | ||
--color-black: #000000; | ||
--color-green: #4fd374; | ||
--color-red: #f97e7e; | ||
--color-blue: #7e99f9; | ||
|
||
--font-large: 48px; | ||
--font-medium: 38px; | ||
--font-regular: 24px; | ||
--font-small: 18px; | ||
--font-micro: 14px; | ||
|
||
--weight-bold: 700; | ||
--weight-semi-bold: 600; | ||
--weight-regular: 500; | ||
|
||
--size-border-radius: 4px; | ||
--animation-duration: 250ms; | ||
} |
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 이거 하자하자 해놓고 귀찮아서 시도 조차 안했는데 막상 이렇게 해놓으면 깔끔하고 좋네요
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.
민우님 과제 하느라 고생 많으셨습니다!
코드 정말 잘 짜셨습니다. 유틸성 함수를 잘 활용하시고, ellipsis 처리나 글씨체 같은 디테일들을 챙기시는 점에서 자극을 많이 받았습니다.
다음 과제도 열심히 달립시다😊
history.pushState(null, null, `/`); | ||
} | ||
|
||
removeFromSpreadDoucmentList(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.
Doucment 오타 있습니당👀💡
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 documentListElement = createDomElementWithId( | ||
"div", | ||
"sideBar_documentList" | ||
); |
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.
dom을 생성할 때 유틸성 함수를 사용하셨군요. 저도 잘 활용해야겠습니다👍
if (className === "spreadButton") { | ||
toggleSpreadIcon(e.target); | ||
toggleDisplay(documentItem.querySelector(".childDocumentList")); | ||
toggleToSpreadDoucmentList(id.toString()); |
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.
toggleToSpreadDoucmentList 함수 내에서 형변환을 하는 게 어떨까요? 아래 onAddChildDocument 함수에서는 id 값이 그대로 들어가는데 toggleToSpreadDoucmentList 함수에서는 id.toString()으로 넘기니까 왜 타입이 달라지는지 궁금해지네요✨
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의 id값은 Number type인데, localStorage에 이를 저장하면 String으로 바뀌어서 타입을 다르게 해줬습니다. 원지님 말대로 toggleToSpreadDoucmentList 함수 내에서 형변환을 하는 게 더 좋을 것 같네요. 감사합니다 ㅎㅎㅎ
if (this.state.selectedDocumentId) { | ||
setTempDocumentToStorage(this.state.selectedDocumentId, { | ||
title: e.target.value, | ||
content: contentElement.value, | ||
tempSaveDate: new Date(), | ||
}); | ||
|
||
if (timerForTitle != null) { | ||
clearTimeout(timerForTitle); | ||
} | ||
timerForTitle = setTimeout(async () => { | ||
await modifyDocumentAPI( | ||
this.state.selectedDocumentId, | ||
e.target.value, | ||
contentElement.value | ||
); | ||
removeTempDocumentFromStorage(this.state.selectedDocumentId); | ||
updateSideBar(); | ||
}, 2000); |
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.
민우님 완성도 높은 코드 잘 보았습니다! 나중에 배포까지 해주시면 또 놀러올게요!
history.pushState(null, null, `/`); | ||
} | ||
|
||
removeFromSpreadDoucmentList(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.
오 원지님 예리하시네요..! 하지만 민우님은 결국
고치지 않으셨다고 합니다...
await deleteDocumentAPI(documentId); | ||
this.route(); |
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 deleteDocumentAPI(documentId); | |
this.route(); | |
deleteDocumentAPI(documentId).then(() => { | |
this.route(); | |
}); |
이건 어떨까요?
if (className === "spreadButton") { | ||
toggleSpreadIcon(e.target); | ||
toggleDisplay(documentItem.querySelector(".childDocumentList")); | ||
toggleToSpreadDoucmentList(id.toString()); | ||
} else if (className === "addChildDocumentButton") { | ||
onAddChildDocument(id); | ||
} else if (className === "deleteDocumentButton") { |
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.
바...반영하셨나요?
@@ -0,0 +1,8 @@ | |||
export const MAIN_PAGE_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.
오 전역적으로 활용되는 String 리터럴이나 그걸 만드는 함수를 이렇게 export 해서 쓰시는군요..! 대박입니다
📌 과제 설명
notion-clone.mp4
🗒️컴포넌트 구조
👩💻 요구 사항과 구현 내용
기본 기능 구현
추가 기능 구현
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점