-
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 #28
base: 3/#4_chunwook
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.
안녕하세요, 천욱님! 노션 클로닝 프로젝트 수고 많으셨습니다!
정말 모든 부분에 많은 정성을 쏟으신 것이 코드와 배포 링크를 보면서 느껴지네요 👍
404페이지를 보여줄 때 기존 컴포넌트들을 "display:none"으로 임시방편으로 가려놓았는데 괜찮은지 리뷰어님들 의견 듣고 싶습니다.
스타일 코드를 이용해서 이미 렌더링된 부분을 가리는 것보다는 아예 렌더링을 하지 않는 게 맞다고 생각합니다. 어디까지나 불필요한 렌더링이고 성능을 생각한다면 다른 방법을 찾으시는 게 맞을 것 같아요.
$homeContainer.className = "post-edit-container"; | ||
$postListContainer.className = "post-list-container"; | ||
$postEditContainer.className = "post-edit-container"; |
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.
저는 클래스를 줄 때 classList.add
를 이용했는데 이렇게 설정하는 방법도 있군요
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.
className을 이용하면 안에있는 클래스는 모두 지우고 정의하는 방법이라서 되도록 사용하지 않습니다.
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.
className을 이용하면 안에있는 클래스는 모두 지우고 정의하는 방법이라서 대도록 사용하지 않습니다.
대도록 (x) 되도록 (o)
맞아요! 보통 한개의 class만 지정해줄때 사용합니당
@@ -0,0 +1,19 @@ | |||
const API_END_POINT = "https://kdt-frontend.programmers.co.kr"; |
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.
vercel 환경변수에 넣으려고 시도했었는데 생각처럼 잘 안돼서 ㅠㅠ gitignore라도 해야겠네요
$target.appendChild($navHeader); | ||
|
||
this.render = () => { | ||
$navHeader.innerHTML = $postsPageHeader(); |
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.
templates에 코드를 빼두니 컴포넌트가 확실히 깔끔해지기는 하네요!
this.setOpenedLists = (nextState) => { | ||
this.state.openedId = [...nextState]; | ||
setItem(OPENED_LIST_KEY, this.state.openedId); | ||
}; |
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,52 @@ | |||
@import "./vars.scss"; | |||
|
|||
@mixin base-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.
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.
scss의 가장 큰 장점인것같아요! 알아두면 정말 많이 쓰일것같습니당 ㅎㅎ
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.
@mixin을 잘 많이 활용해서 좋네요 ^^
const $postListContainer = document.createElement("div"); | ||
const $homeContainer = document.createElement("div"); | ||
const $postEditContainer = document.createElement("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.
App에서 이러한 Container들을 만들어주신 이유를 여쭤봐도 될까요?
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.
저 코드가 display:none을 하기위해서 추가된 코드에요.
처음엔 각각 렌더링을 하려고 했는데 App에서 pathname에 따라 렌더링 되는부분이 잘 안되더라고요..
404페이지에서 list가 보인다거나 그런 현상이 발생해서 임시방편으로 저렇게 넣어둔겁니다ㅎ.ㅎ
저도 혜준님 생각대로 불필요한 렌더링이 발생한다고 생각해서 저 부분은 변경하도록 하겠습니다~~
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.md
Outdated
- 코드 리뷰 반영 기간 : 2022년 11월 23일(수) ~ 2022년 11월 25일(금) | ||
- 내용 | ||
- **Day 17 [프로젝트] 노션 클로닝 요구사항** 확인 부탁드립니다. | ||
(작성중...) |
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 (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.
호출 실패하면 안될텐데!!!!!!
}; | ||
|
||
const formatTime = (time) => { | ||
if (isNaN(time)) 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 $home = () => { | ||
return ` | ||
<div class="home"> | ||
<h1>🔳 JooNotion</h1> |
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 $onLoadParentList = (posts, selectedPostId, openedId) => { | ||
if (Array.isArray(posts)) { | ||
return ` | ||
<ul> | ||
${posts | ||
.map((post) => { | ||
const isParentIncluded = openedId.includes(post.id.toString()); | ||
return ` | ||
<li class="post-list"> | ||
${$listContent(post, selectedPostId, openedId)} | ||
${ | ||
post.documents.length > 0 | ||
? `${$onLoadChildList( | ||
post.documents, | ||
selectedPostId, | ||
openedId, | ||
isParentIncluded | ||
)}` | ||
: `${$emptyPage(isParentIncluded)}` | ||
} | ||
</li> | ||
`; | ||
}) | ||
.join("")} | ||
</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.
아니 여기 왜이렇게 못생겼어요?? 코드 tabwidth 수정이 필요해보여요
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.
안녕하세요 천욱님~~ 마지막 코드리뷰 하러 왔습니다!
과제하시느라 수고 많으셨어요!
vercel로 배포까지 하셔서 실행해 봤는데 기능구현도, css도 너무 잘하셨네요 👍
다음 과제도 화이팅 입니다~
this.init = async () => { | ||
const { pathname } = window.location; | ||
if (pathname === "/") { | ||
$postEditContainer.style.display = "none"; | ||
$homeContainer.style.display = "flex"; | ||
} else if (pathname.indexOf("/documents/") === 0) { | ||
$postEditContainer.style.display = "flex"; | ||
$homeContainer.style.display = "none"; | ||
|
||
const [, , id] = pathname.split("/"); | ||
if (id === "new") { | ||
postEdit.setState({ | ||
...this.state, | ||
id, | ||
}); | ||
} else { | ||
const post = await request(`/documents/${id}`, { | ||
method: "GET", | ||
}); | ||
postsPage.setState(post); | ||
postEdit.setState(post); | ||
} | ||
} else { | ||
notFound.render(); | ||
$postListContainer.style.display = "none"; | ||
$postEditContainer.style.display = "none"; | ||
$homeContainer.style.display = "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.
pathname에 따라 렌더링을 이해하기 쉽게 잘 작성하신 것 같아요~
if (confirm(`제목 : ${post.title}\n해당 글을 삭제할까요?`)) { | ||
const res = await request(`/documents/${post.id}`, { | ||
method: "DELETE", | ||
}); | ||
if (res.id) { | ||
this.setState(); | ||
postList.setOpenedLists(openedId.filter((item) => item !== id)); | ||
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.
삭제하기 전에 확인하는 거 좋네요!
export default function NotFound({ $target }) { | ||
const $notFound = document.createElement("div"); | ||
$notFound.className = "not-found"; | ||
|
||
$target.appendChild($notFound); | ||
|
||
this.render = () => { | ||
$notFound.innerHTML = ` | ||
<h1>페이지를 찾을 수 없습니다. 😢</h1> | ||
<button>홈으로 이동</button> | ||
`; | ||
}; | ||
|
||
this.init = () => { | ||
$notFound.addEventListener("click", (e) => { | ||
const $button = e.target.closest("button"); | ||
if ($button) { | ||
window.location = window.location.origin; | ||
} | ||
}); | ||
}; | ||
|
||
this.init(); | ||
} |
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.
404 처리~~ 좋아요
<div class="post-date-container"> | ||
<span>생성날짜 : ${formatTime(new Date(createdAt))}</span> | ||
<span>수정날짜 : ${formatTime(new Date(updatedAt))}</span> | ||
</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.
생성/수정 날짜 보여주는거 너무 좋네요
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.
안녕하세요 천욱님~ 프로젝트 하느라 고생 많으셨습니다~
$postEditContainer.style.display = "none"; | ||
$homeContainer.style.display = "flex"; | ||
} else if (pathname.indexOf("/documents/") === 0) { | ||
$postEditContainer.style.display = "flex"; | ||
$homeContainer.style.display = "none"; | ||
|
||
const [, , id] = pathname.split("/"); | ||
if (id === "new") { | ||
postEdit.setState({ | ||
...this.state, | ||
id, | ||
}); | ||
} else { | ||
const post = await request(`/documents/${id}`, { | ||
method: "GET", | ||
}); | ||
postsPage.setState(post); | ||
postEdit.setState(post); | ||
} | ||
} else { | ||
notFound.render(); | ||
$postListContainer.style.display = "none"; | ||
$postEditContainer.style.display = "none"; | ||
$homeContainer.style.display = "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.
이런 style 부분은 className on/off 하는 방법은 어떨까요?
@@ -0,0 +1,52 @@ | |||
@import "./vars.scss"; | |||
|
|||
@mixin base-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.
@mixin을 잘 많이 활용해서 좋네요 ^^
${time.getFullYear()}-${String(time.getMonth() + 1).padStart(2, "0")}-${String( | ||
time.getDate() | ||
).padStart(2, "0")} | ||
/ ${String(time.getHours()).padStart(2, "0")} : ${String( | ||
time.getMinutes() | ||
).padStart(2, "0")}`; |
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.
00:00 이 형식으로 시간 출력하고 싶으실때 쓰면 좋을것 같아요~~
<div class="overlay"></div> | ||
<div class="modal"> | ||
<div class="modal-title"> | ||
<h2>📝오늘하루 어땠나요?</h2> |
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.
이런 감정적인 prompt 좋네요 ^^
* @returns | ||
*/ | ||
const $editPost = ({ title, content, createdAt, updatedAt }) => { | ||
content = content === null ? "" : 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.
$ prefix는 보통 element류에 붙는 것인데 템플릿 함수 이름으로 쓰기에는 조금 애매한 것 같기도 합니다.
createEditPostTemplate 또는 getEditPostTemplate 같은 이름이 어떨까 싶네요
혹은 템플릿을 import * as Templates from './templates'
같은 식으로 임포트해서
Templates.getEditPost({ ... })
처럼 쓸 수도 있겠습니다.
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.
아하~ 감사합니다 :) 반영할게요!
* @param {*} posts | ||
* @returns | ||
*/ | ||
const $listContent = (post, selectedPostId, openedId) => { |
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.
템플릿마다 인자가 오브젝트이기도 하고 position argument이기도 해서, 하나로 통일하는게 좋을 것 같아요.
보통은 순서와 무관하게 렌더링될 수 있도록 오브젝트를 많이 사용합니다. (예: https://underscorejs.org/#template)
전체적으로 모달 등 기능도 많이 구현되었고, 디테일에 신경쓰신 부분들이 눈에 띄어 좋네요. ^^ |
📌 과제 설명
Vanilla JS로 만든 노션 클로닝
(추후 리팩터링 예정)
JooNotion Demo
👩💻 요구 사항과 구현 내용
기본 요구사항
바닐라 JS만을 이용해 노션을 클로닝합니다.
기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.
보너스 요구사항
그외 개선하거나 구현했으면 좋겠다는 부분이 있으면 적극적으로 구현해봅니다!
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점