-
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 #11
base: 3/#4_shindongwoo
Are you sure you want to change the base?
[Mission4/신동우] - Project_Notion_VanillaJS #11
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.
안녕하세요.
코드 하나하나를 보기보다는 실제 페이지를 보면서 작성하였습니다 👍
디자인 부분
개발관련
- 제목없음으로 신규생성 후 삭제하려고 하였을때 에러가 발생합니다. 확인해보니 DELETE 요청을 보내야하는데, GET 요청을 보내고 있네요. (제목없음의 경우 페이지도 열리지 않습니다.)
궁금한 점
- 계속해서 진행하면서 저는 commit 단위를 나누는게 제일 문제가 많다고 생각했습니다.
그냥 무작정 기능들을 구현하다가 넘어가는 경우들도 많아지고, 어느 면에선 이정도면 커밋을 해도 되나 싶은 부분이 많아서 발전이 필요할 것 같습니다. 어떤식으로 나누는 것이 좋을까요?
처음부터 커밋 메시지를 적어놓고 개발하는 것도 하나의 방법일 수 있습니다. 그렇게 될 경우 미리 작성된 메시지의 기능 이외에는 작업을 하지 않을 것이고, 자연스럽게 커밋이 기능 단위로 쪼개질 수 있습니다 👍
- sideBar의 List에 어떤 state에 변화가 오면 해당 List의 innerHTML을 localStorage에 담아 통째로 바꿔줍니다. 좀 더 나은 방법이 있을까요?
=> 저는 지금 Virtual DOM을 적당히 구현해보자는 마인드로 낙관적 업데이트와 localStorage를 통해 유지 하려고 하고 있습니다.
=> 어떠한 방법으로 하는 것이 좋을지 궁금합니다!
음...낙관적 업데이트는 사용하는것이 맞을 것 같고, 변경된 부분에 대해서만 바뀌는게 제일 best이긴 한데 구현하기가 까다로울것 같습니다 ㅠㅠ
- 폴더 구조를 한번 바꿔보았는데 너무 내멋대로 바꾼 것 같아서 어떤식으로 발전해야될지 궁금합니다!
멋대...로 바꾼 느낌은 아닌것같습니다 :) 다만,
router
와 같은 부분들은 lib에 포함되지 않을 것 같아요.
📦 src
┣ 📂 Styles: 공통css 부분을 관리하는 폴더(Theme)
┃ ┣ 📜 default.scss
┃ ┣ 📜 global-styles.js
┃ ┗ 📜 theme.ts
┣ 📂 components: 컴포넌트 파일들이 위치하는 폴더
┃ ┣ 📂 General
┣ 📂 utils
┣ 📂 hooks
┣ 📂 lib
┣ 📂 pages
┃ ┗ 📜 Routing을 위한 페이지 파일들이 위치하는 폴더
┣ 📜 App.tsx
┗ 📜 index.tsx
- 제일 궁금했던 점입니다. state를 모두 App으로 끌고 와서 뿌려주면 re-rendering이 되는데 이렇게 하는 것이 옳은 방법인가요?
=> 계속해서 state를 올려주고, 함수를 내려주고 하는 것이 불필요하다고 느껴지는 부분들이 많아서 고민입니다..ㅜ
선택된 부분에 대해서 별개로 api를 날릴 수 있다면 리스트업되는 state와 선택된 state를 별개로 가져간다던지 추가적인 조치가 있을 수는 있을 것 같습니다. 아마 react를 사용했다면 더 효율적으로 조치할 수 있었을 것 같습니다 👍
this.render = () => { | ||
$modalHeader.innerHTML = ` | ||
<button class="full-screen-btn" id="fullScreen"> | ||
<svg |
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.
svg는 별도로 파일 분리해서 가져오는게 더 좋지 않았을까요??
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 onClose = () => { | ||
$modalContainer.classList.remove('show-modal'); |
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에 접근했다는게 이 부분이시군요 :)
$target, | ||
initialState: {}, | ||
// 타이틀이 없거나 제목 없음일 경우 리스트에서 삭제. | ||
clearUntitledDocument: () => { |
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.
동우님 전체적으로 코드를 신경써서 작성했다는게 느껴져서 너무 보기 좋았습니다. 동우님의 코드를 보고 제가 과제를 수행할 때 해결하지 못한 부분들에 대한 해결책을 보고 배울 수 있었고, 주석도 하나 하나 신경써서 달아 놓은 것을 제 스스로 많이 뒤돌아 보게 되지 않았나 싶습니다. 좋은 코드 보고 많이 배우겠습니다. 감사합니다.
export const $ = (selector, target = document) => { | ||
return target.querySelector(`${selector}`); | ||
}; | ||
|
||
export const $all = (selector, target = document) => { | ||
return target.querySelectorAll(`${selector}`); | ||
}; | ||
|
||
export const $createElement = (element) => { | ||
return document.createElement(`${element}`); | ||
}; |
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 = () => { | ||
$modalHeader.innerHTML = ` | ||
<button class="full-screen-btn" id="fullScreen"> | ||
<svg |
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 적용과 배포까지 완벽합니다.. 주석도 꼼꼼히 달아서 이해가 잘되었고 저도 앞으로 주석을 잘 달아야겠다고 느꼈어요. 고생하셨습니다!
export const validateState = (state) => { | ||
return state !== undefined && state !== 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.
state
에 대한 vaildation
까지 처리해주신 모습 본받아야할 것 같아요! ㅎㅎ
}; | ||
|
||
export const addDocumentToList = (target, documents) => { | ||
target.insertAdjacentHTML( |
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.
insertAdjacentHTML
라는 함수는 처음 보는 것 같네요..! 덕분에 한 번 찾아보게 되었습니다 ㅎㅎ
} | ||
// 삭제한 document의 자식 document 아래로 옮기기 | ||
const $newUl = $('ul', $list); | ||
if (documents?.length) { |
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.
?.
이 뭔지 몰라서 찾아보니 옵셔널 체이닝이라는 문법이군요! 사용법 배우고 갑니다 ㅎㅎ
📌 과제 설명
VanillaJS Notion Project
👩💻 요구 사항과 구현 내용
배포 링크: https://dongtion.netlify.app/
시연 영상
default.mov
기본 요구사항
추가 요구사항
✅ PR 포인트 & 궁금한 점
궁금한 점
그냥 무작정 기능들을 구현하다가 넘어가는 경우들도 많아지고, 어느 면에선 이정도면 커밋을 해도 되나 싶은 부분이 많아서 발전이 필요할 것 같습니다. 어떤식으로 나누는 것이 좋을까요?
=> 저는 지금 Virtual DOM을 적당히 구현해보자는 마인드로 낙관적 업데이트와 localStorage를 통해 유지 하려고 하고 있습니다.
=> 어떠한 방법으로 하는 것이 좋을지 궁금합니다!
=> 계속해서 state를 올려주고, 함수를 내려주고 하는 것이 불필요하다고 느껴지는 부분들이 많아서 고민입니다..ㅜ