[Mission4/최은지] - Project_Notion_VanillaJS#20
[Mission4/최은지] - Project_Notion_VanillaJS#20dmswl98 wants to merge 25 commits intoprgrms-fe-devcourse:3/#4_choieunjifrom
Conversation
This reverts commit 064c959.
rjsduf0503
left a comment
There was a problem hiding this comment.
안녕하세요 은지님 :)
이번 노션 프로젝트도 수행하느라 고생 많으셨습니다 ㅎㅎ
어느새 팀원으로써 마지막 코드 리뷰를 남기게 되었는데 상당히 아쉽네요!
rich editor를 정말 사용성 있게 잘 만드셨더라구요! 한 수 배우고 갑니다 ㅎㅎ
| } else if (pathname.indexOf("/documents/") === 0) { | ||
| const [, , documentId] = pathname.split("/"); | ||
|
|
||
| const documentData = await getDocumentContent(documentId); |
There was a problem hiding this comment.
p6) document의 내용을 가져오는 함수 getDocumentContent의 반환값을 저장하는 변수이니 documentContent라는 변수명이 더 적당하지 않을까라는 의견을 남깁니다!
There was a problem hiding this comment.
오호 생각치 못했던 부분..! documentContent 네이밍이 더 좋은 것 같아요! 꼭 반영하겠습니당
|
|
||
| if (state === STATE.OPEN) { | ||
| if (openedList[id]) { | ||
| delete openedList[id]; |
There was a problem hiding this comment.
오.. 저는 array의 delete만 생각해서 제거하더라도 배열에 빈 값이 남게되어 길이가 그대로라 사용하지 않고 직접 제거해주는 방식을 사용했는데 Object 형식이라 delete로 제거하면 정상적으로 제거가 되는군요.. 배우고 갑니다!
p6) 처음에 OPENED_LIST라고 되어 있어서 리스트 형태인 줄 알았어요! OPENED_LIST 대신 다른 이름을 사용하는게 어떨까요? 예를 들면.. 음.. OPENED_DOCOUMENTS ?
|
|
||
| if (documents.length > 0) { | ||
| deleteDocuments(documents); | ||
| deleteDocumentId.reverse(); |
There was a problem hiding this comment.
ask) reverse를 사용한 이유는 최하단부터 전부 삭제하기 위함인가요? 제가 과제를 이해하기로는 하위 문서가 있는 상위 문서를 삭제했을 때 하위 문서까지 삭제하는건 아니었던 것 같아서 질문드립니다!! :)
There was a problem hiding this comment.
맞습니다! 노션은 상위 문서를 삭제하면 하위 문서까지 삭제되길래 구현해보았는데 오호... 구현 사항이 아니였던건가요?ㅋㅋㅋ 😵
| if (type === "title") { | ||
| onEditTitle(id, data); | ||
| } else if (type === "content") { | ||
| onEditContent(id, data); | ||
| } |
There was a problem hiding this comment.
p6) 추후 class에 다른 클래스가 추가될 경우를 대비해 contains를 사용하는게 어떨까요? :)
Kal-MH
left a comment
There was a problem hiding this comment.
은지님! 벌써 마지막 코드리뷰라니 너무 아쉽습니다ㅠ
같이 공부할 수 있어서 너무 즐거웠던 한 달이었고요 할로윈 고스트 마스크 쓰고 있던 게 새록새록(?) 생각이 나네요ㅎㅎ
사소하지만 동작 리뷰건으로,
전체 텍스트에서 대해서 뒤에서 앞으로 드래그를 했을 때, 모달창이 뜨지 않는 것 같더라고요
알고계시면 좋을 거 같습니다.
디자인도 그렇고 노션을 그대로 클론하려고 노력하신 거 같아요 노션의 기능들이 이렇게도 구현될 수 있겠구나 생각이 들어서 잘 보았습니다
수고 많으셨어요:)!
| @@ -0,0 +1,29 @@ | |||
| import { request } from "./index.js"; | |||
There was a problem hiding this comment.
이렇게 api함수들을 따로 정리하니까 보기 좋네요!
ask) request가 정의된 파일 이름이 왜 index.js인지 물어봐도 될까요:)?
| return res.json(); | ||
| } catch (err) { | ||
| console.error(err); | ||
| history.go(-1); |
There was a problem hiding this comment.
p5) 에러가 발생되었을 때, history.go(-1)을 통해서 한 단계전으로 돌아가는 군요!
사실 SidebarNav에서 onDeleteDocument()를 통해서 문서를 삭제했을 때, 삭제한 문서의 id를 통해서 push()를 호출하는 것으로 보입니다.
삭제된 문서를 조회하는 과정에서 request error가 발생했고, history.go(-1)를 통해서 존재하는 문서까지 거슬러 올라가면서(이해한 게 맞나요?) App.js에서 다수의 에러메세지가 뜨는 것을 발견했어요! 삭제를 할 때마다 에러메세지가 계속 쌓일 거 같은데, 사용자 입장에서는 잘 보이지 않고 동작도 잘 이뤄지기 때문에 넘어가도 되는 부분인 것도 같습니다!
고민이 되는 부분이네요 어떻게 생각하시나요:)?
There was a problem hiding this comment.
history.go(-1)는 딱 이전 페이지를 보여주는데 만약, 문서들을 조회하고 조회한 문서들이 삭제되면 이전 페이지에 해당하는 문서가 삭제된 경우라 계속 에러가 발생하는 것 같아요. 저장된 데이터에는 삭제된 문서가 없어 fetch할 수 없기에... 이 부분은 승준님 말씀처럼 루트로 되돌리는 것이 나을 것 같고 꼭 반영해야겠어요!
| $target: $document, | ||
| initialState, | ||
| onEditTitle: (id, data) => { | ||
| debounce(async () => { |
There was a problem hiding this comment.
debounce를 따로 모듈로 빼서 동작하게 하는군요 저도 바로 적용해야겠습니다:)
| return ""; | ||
| }; | ||
|
|
||
| export const renderTextStyleMenu = () => { |
There was a problem hiding this comment.
모달창에 대해서 이렇게 표현할 수도 있군요:) 또 하나 배워갑니다. ㅋㅋ
There was a problem hiding this comment.
배포한 링크를 올려주셔서 테스트 해보기 편했네요! 모달창을 뜨게 해서 rich content 구현한 게 너무 멋있습니다! 한 번 드래그 해봤는데 진짜 노션처럼 떠서 진심으로 놀랬어요...
그리고 몇몇 문서를 보면서 은지님의 광기를 느낄 수 있었습니다... 오프라인에서 진정한 광기를 못 보아 아쉽네요... ㅋㅋㅋㅋㅋㅋ
제 생각엔 충분히 컴포넌트가 잘 분리된 것 같습니다. XSS 대응방안으로는 지금 딱 떠오르는 건... React로 구현하기...? ㅋㅋㅋㅋㅋㅋ JSX를 사용하니까... ㅎㅎㅎㅎ... 무지한 저의 지식에 다시 한번 자책감을 느낍니다.🙂
5주간 저 때매 고생하셨습니다😀 종종 코드 훔치러 오겠습니다. ^^
| return res.json(); | ||
| } catch (err) { | ||
| console.error(err); | ||
| history.go(-1); |
There was a problem hiding this comment.
P5 : 오호 뒤로가기 처리를 하는 군요!
만약에 제가 구글로 들어간 후, 잘못된 주소로 접속, 예를 들면 새로운 탭을 키고 https://dmswl98-notion-clone-project.netlify.app/documents/00101과 같이 잘못된 documentId로 들어가면 구글로 튕기네요! 사용자 입장에선 not found가 떠서 잘못된 주소임을 가르켜 주는 것이 아니라 뒤로 튕겨버리니 익숙치 않은 경험이 될 수 있을 것 같습니다! history.push('/') 을 통해 홈으로 리다이렉팅 시키는 것은 어떨까요?
There was a problem hiding this comment.
저도 그 점을 인지하고 있었는데 까먹고 고치지 못했네요ㅎㅎ 꼭 반영해야겠군요 🤯
| }"></div> | ||
| </div> | ||
| ${renderChildDocuments(documents)} | ||
| `; |
There was a problem hiding this comment.
함수명만으로도 무엇을 할 지 알게되니까 파악하기 되게 쉬워지네요!
| `; | ||
| }; | ||
|
|
||
| const closeSidebar = (e) => { |
There was a problem hiding this comment.
sidebar 닫히고 열리는 것까지 ㄷㄷ... 토글 버튼 돌아가는 것도 오프라인 때 구현하시는거 봤었는데 CSS랑 JS를 잘 이용하시는 것 같아 부럽습니다!
| $nav.innerHTML = ` | ||
| <ul class="document-list"> | ||
| ${createDocument(this.state, 0)} | ||
| </ul>`; |
There was a problem hiding this comment.
P5 : $nav.innerHTML = ""; 이거 없어도 되지 않을까요!? 어차피 덮어씌워지는 거니까 없어도 될 것 같다는 생각이 들었어요.😀
| export const DEFAULT_TEXT = { | ||
| TITLE: "제목 없음", | ||
| CONTENT: "내용을 입력하세요.", | ||
| }; |
eastroots92
left a comment
There was a problem hiding this comment.
11월 26일 12시에 줌을 통해 구두로 피드백을 드렸고
기록 남기기용으로 구두로 이야기 나눴던 코드리뷰를 간략하게 적어두려 합니다.
CSS 관련
-
Reset CSS와 normalize CSS
가능하다면 의도적으로 넣어둘 것
JS 관련
-
클로저 패턴 + 관심사 별로 묶기
[vanilla-js-notion/api.js at feature · gxxrxn/vanilla-js-notion](https://github.com/gxxrxn/vanilla-js-notion/blob/feature/src/utils/api.js)
[vanilla-js-notion/storage.js at feature · gxxrxn/vanilla-js-notion](https://github.com/gxxrxn/vanilla-js-notion/blob/feature/src/utils/storage.js)
-
CustomEvent / 옵저버 패턴 / 펍섭패턴
기술 구조 관련
-
폴더 구조 잡기 (도메인 지역성을 고려한 구조, 기능 별로 모아두는 구조)
-
API
- ky, axios 등을 많이 사용함.
-
역할을 명확하게 하기
-
상수
- 전역에서 알아야 하는건가? 딱 그 곳에서만 알면 좋은 건가?
-
컴포넌트
- customElements ⇒ 커스텀한 컴포넌트를 만들 수 있다.
- container-component
const template = document.createElement('template'); template.innerHTML = ` <style> h3{ color:orange; } </style> <div class="user-card"> <h3></h3> </div> ` class UserCard extends HTMLElement{ constructor(){ super(); //shadow DOM this.attachShadow({mode:'open'}); this.shadowRoot.appendChild(template.content.cloneNode(true)); this.shadowRoot.querySelector('h3').innerText = this.getAttribute('name') // this.innerHTML = `<h3>${this.getAttribute('name')}</h3>` } } window.customElements.define('user-card', UserCard) <body> <user-card>안녕</user-card> </body>
-
기능(function)과 화면(presentation)
- api 에서 try catch 등을 통해 alert를 띄우는건 좋지않음.
import { push } from '../router.js'; import { API_END_POINT } from '../url.js'; import { USER_NAME } from './constants.js'; export const request = async (url, options = {}, data) => { const res = await fetch(`${API_END_POINT}${url[0] === '/' ? url : `/${url}`}`, { ...options, headers: { 'x-username': USER_NAME, 'Content-Type': 'application/json', }, body: JSON.stringify(data), }); if (res.ok) { return await res.json(); } else { push('/'); throw new Error(`${res.status} Error`); } }; const getData({params}, callback);
-
-
Router 처리
- 역할 위임 (queryParams, url path)
- 관리하는법
const [, , documentId] = pathname.split("/"); // 이렇게 하면 URL 구조를 이해하기 쉽지 않음. 추가로 URL이 어려움.
HTML 관련
- 시맨틱 태그
기타
-
eof 이슈
editorConfig 등을 사용해서 지켜줄 것
-
XSS 이슈
📌 과제 설명
바닐라 자바스크립트로 노션 구현하기
👩💻 요구 사항과 구현 내용
배포 주소
https://dmswl98-notion-clone-project.netlify.app/
기본 요구사항
추가 요구사항
추가적으로 구현한 사항
개선할 사항
✅ PR 포인트 & 궁금한 점