-
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 #34
base: 3/#4_choibyeol_working
Are you sure you want to change the base?
[Mission4/최별] - Project_Notion_VanillaJS #34
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.
안녕하세요 별님! 같은 유리팀 유지영입니다👍
직접 실행을 해보니, 삭제 버튼을 눌렀을 때 루트 document로 돌아가지만 새로고침하지 않으면 편집기에 적용이 안되더라구요! 이 부분도 추가되면 더욱 좋을 것 같아요 :)
첫 개인 프로젝트 너무 수고하셨습니다! 끝까지 마무리하셔서 과제 제출하시는 모습이 감동받았습니다><
|
||
.add-btn { | ||
position: absolute; | ||
padding: 0.5rem 1rem; |
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.
add-btn 과 delete-btn의 padding이 겹쳐있어서 + 버튼을 누르면 삭제버튼이 눌리는 경우가 생기네용!! padding값을 조절해주면 좋을 것 같아요! 😊
if (isNew) { | ||
const createdPost = await request("/documents", { | ||
method: "POST", | ||
body: JSON.stringify(post), | ||
}); | ||
history.replaceState(null, null, `/documents/${createdPost.id}`); | ||
removeItem(postLocalSaveKey); | ||
|
||
this.setState({ | ||
postId: createdPost.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.
새 페이지를 추가하거나 하위문서가 추가되면 바로 postId로 주소를 넘겨서 postId가 new일 때의 로직은 사용되지 않아도 될 것 같아요! 😊
editor.setState( | ||
this.state.post || { | ||
title: "", | ||
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.
동일한 문서를 재클릭했을 때 this.state.post 값이 undefined여서 기존의 제목과 내용이 뜨지 않는 것 같아요!
if(this.state.post){
editor.setState(this.state.post)
}
이런 식으로 undefined일 땐 아예 setState가 일어나지 않도록 하는 것도 방법일 것 같아요! 👍
|
||
$sidebarBody.addEventListener("click", (e) => { | ||
const target = e.target; | ||
const dataId = target.closest("li").dataset.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.
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 { postId } = this.state; | ||
|
||
if (postId !== "new") { | ||
const post = await request(`/documents/${[postId]}`); |
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.
postId가 배열로 들어가는 이유가 따로 있을까용?? 😊
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; | ||
$editor.querySelector("[name=title]").value = this.state.title; | ||
$editor.querySelector("[name=content]").innerHTML = this.state.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.
페이지의 내용을 수정하고 다른 list를 누르면 content 내용이 해당 list의 content가 아니라 수정했던 이전 list의 content가 계속 떠 있더라구요! 아마 이 부분이 value가 아니라 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.
헉 지영님 고마워요...!!! 역시 똑똑이 지영님🥺
<html> | ||
<head> | ||
<title>Star notion</title> | ||
<link rel="stylesheet" href="src/style/style.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.
특정 document를 클릭하고 새로고침을 하면 css가 날라가서 찾아보니! css도 스크립트처럼 상대경로가 아닌 절대경로로 지정해주니 해결되는 것 같네요! 😊
<link rel="stylesheet" href="/src/style/style.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.
와 그렇구나 ㅠㅠㅠ 절대 경로로 지정해주는 습관 들여야겠어요...
// const fetchNewPost = async (postId) => { | ||
// push(`/documents/${postId}`); | ||
// }; |
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.
주석은 삭제하는 습관을 들이는게 좋겠어요! git이 관리해주니까요:-)
/* 연속으로 입력을 하고 있을 때는 계속 이벤트 발생을 지연시키다가 | ||
입력을 멈췄을 때, 즉, 마지막으로 이벤트가 발생하고 일정 시간이 지났을 때 | ||
지연시켰던 이벤트를 실행시키는 것 - 디바운스 | ||
디바운스를 이용하면 이벤트 발생하는 횟수를 줄일 수 있다. -> 성능, 최적화 | ||
*/ |
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.
이러한 주석은 readme에 작성하면 좋을 것 같습니다ㅎㅎ
$addBtn.innerHTML = "+"; | ||
const $deleteBtn = document.createElement("button"); | ||
$deleteBtn.className = "delete-btn"; | ||
$deleteBtn.innerHTML = "x"; |
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.
이부분도 textContent로 처리할 수 있었을 것 같네요ㅎㅎ
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, body, footer로 쪼갠 점이 인상 깊었고, 지영님께서 엄청 꼼꼼히 달아주셔서 저는 살짝 수정하면 좋을 내용 적고 갑니다~
수고 많으셨습니다 :)
export default function PostEditPage({ $target, initialState }) { | ||
const $postEditPage = document.createElement("div"); | ||
$postEditPage.className = "post-edit-page"; | ||
|
||
this.state = initialState; | ||
|
||
let postLocalSaveKey = `temp-post-${this.state.postId}`; | ||
|
||
const post = getItem(postLocalSaveKey, { |
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 방어코드 처리를 추가해주시면 좋을 것 같습니다 :)
/* | ||
sidebar 배경색: rgb(250, 250, 249) | ||
sidebar hover 색: rgb(230, 230, 230) | ||
sidebar 눌러진 페이지: rgb(238, 238, 238) | ||
*/ |
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.
프로젝트하느라 고생많으셨습니다!
코드 보면서 간단하게 의견 남겨보았습니다~
this.render = () => { | ||
$target.appendChild($sidebar); | ||
$sidebar.appendChild($sidebarHeader); | ||
$sidebar.appendChild($sidebarBody); | ||
$sidebar.appendChild($sidebarFooter); | ||
}; | ||
|
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를 컨테이너로 하신 것 같은데, 그러면 하위의 sidebarHeader, sidebarBody, sidebarFooter 에 타겟으로 sidebar 컨테이너를 넘겨주기만 해도 이러한 render 함수는 생략할 수 있지 않을까요?
new를 하는 시점에서 sidebar 컨테이너에 append가 되고, 어차피 렌더링은 innerHTML로 처리하니까요! 혹시 이후에 render 함수를 쓰는 일이 있나 봤는데 없는 것 같아서 말씀드려봅니당
setState: this.setState(), | ||
}); | ||
|
||
new SidebarFooter({ | ||
$target: $sidebarFooter, | ||
setState: this.setState(), |
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.
함수를 props로 전달할 때에는 호출하지 않고 넘겨주셔야 더 안전할 것 같아요! this.setState만 넘겨주고 하위 컴포넌트에서 실행이 필요한 경우에만 setState() 로 실행하는 게 맞아 보입니다.
그리고 부모 컴포넌트의 setState를 자식 컴포넌트에서 실행하게 되면 나중에 상태관리가 꼬이게 되니까 다른 방법을 고민해봐야 할 것 같아요. 강의에서처럼 onClick과 같은 트리거 함수를 정의해서 자식에게 전달하고, 그 트리거 함수를 실행하면 부모 컴포넌트에서 setState를 실행하는 게 더 좋지 않을까 의견드려봅니다!
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 목적으로는 각 컴포넌트에서 해주는게 좋을것 같고
상위 setState를 통해 하위 state를 관리하는 방향이 더 좋을것 같습니다~
만약 상위의 상태값을 하위에서 바꿔야하는 로직(이벤트)이 있다면 해당 setState를 가져오는게 아닌 해당 로직을 호출하면서 내부에서 변경하는게 좋을것 같습니다~
//ex
new SidebarFooter ({
onClick : () => {
const nextState = 변경로직
this.setState(nextStatee)
}
})
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.
안녕하세요~ 같은팀 양상우 입니다!
UI적으로 컴포넌트를 분리하고 분리함에 따라 생기는 state관리에 신경을 쓰신게 보입니다!!! 잘 봤습니다!
질문에 대한 답변은 팀원들 전반적으로 고민이 있는 부분 같습니다!
다들 한번 얘기 나눠보는것도 좋을것 같아요~!~!
const $sidebar = document.createElement("div"); | ||
const $postEditPage = document.createElement("div"); | ||
|
||
$target.appendChild($sidebar); | ||
$target.appendChild($postEditPage); | ||
|
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(); | ||
|
||
$editor.addEventListener("keyup", (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.
e를 받아올때 한번에 {target}으로 분리해주는것도 좋을것 같아요~
setState: this.setState(), | ||
}); | ||
|
||
new SidebarFooter({ | ||
$target: $sidebarFooter, | ||
setState: this.setState(), |
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 목적으로는 각 컴포넌트에서 해주는게 좋을것 같고
상위 setState를 통해 하위 state를 관리하는 방향이 더 좋을것 같습니다~
만약 상위의 상태값을 하위에서 바꿔야하는 로직(이벤트)이 있다면 해당 setState를 가져오는게 아닌 해당 로직을 호출하면서 내부에서 변경하는게 좋을것 같습니다~
//ex
new SidebarFooter ({
onClick : () => {
const nextState = 변경로직
this.setState(nextStatee)
}
})
$renderList.innerHTML = ""; | ||
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.
이 부분 로직은 계속 중복 되는것 같습니다!! State 랜더를 담당하는 부분까지 역할이 과중 된것 같습니다
📌 과제 설명
바닐라 JS만을 이용해 노션을 클로닝합니다.
기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.
동작 모습
파일 구조
👩💻 요구 사항과 구현 내용
기본 요구 사항
보너스 요구사항
개선해야 할 부분
새롭게 리뷰 받은 부분
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점