-
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_Vanilla_JS #10
base: 3/#4_kalmihyun
Are you sure you want to change the base?
[Mission4/갈미현] - Project_Notion_Vanilla_JS #10
Conversation
1. constants.js 2. error.js 3. router.js - new CustomEvent 사용 - DOCUMENT CREATE, PUT, DELETE 제공 - ROUTE_PUSH - HEADER_TITLE_CHANGE(에디터 제목 입력창과 사이드바 메뉴의 제목 동기화) 4. storage.js
1. EditPage - Topbar, Header, Editor를 렌더링 - autoSaveDocument()를 호출해서 현재 작업중인 document 자동 저장 - LOCAL_IS_GOING_TO_PUT, LOCAL_IS_WAITING_USER_ANWER, LOCAL_IS_GOING_TO_PUT 세가지 플래그를 이용해서 임계구역 설정. - 사용자가 작성 중 자동저장이 되기 전에 다른 페이지로 이동할 시, confirm()를 통해 의사를 확인한다.
DocumentPage 1. DocumentHeader : 워크 스페이스 문구, 새로 생성 버튼 2. Documents: 전체 문서 목록 트리형태로 보여줌 - 펼침, 삭제, 생성 버튼이 각 문서마다 달려 있다. - 생성의 경우, 낙관적 업데이트 이후, 전체 문서 목록을 다시 렌더링 - 삭제, 생성은 routes.js에 정의된 함수를 호출하여 이벤트를 발생, url과 화면을 변경한다. 3. DocumentFooter : 새로운 문서 생성하기 문구, 새로 생성 버튼
- Editor Topbar에 들어가는 deleteButton으로 클릭시 삭제 이벤트가 발생한다. - 해당 id의 문서 삭제 - 부모 문서가 있으면 해당 부모 id로 url 및 화면 변경 - 부모 문서가 없으면(root document이면) /로 경로 설정
…o Header component
…h Header components
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 styleObj = { | ||
DEFAULT_PADDING: 15, |
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.
자주 쓰이는 default padding도 상수로 처리하셨네요 좋아보입니다~~
src/components/utils/error.js
Outdated
import { properties } from "./constants.js"; | ||
|
||
const { ID, TITLE, CONTENT, DOCUMENTS } = properties; | ||
|
||
//properties | ||
export const hasNewTarget = (target) => (target ? true : false); | ||
|
||
export const hasId = (state) => state.hasOwnProperty(ID); | ||
|
||
export const hasTitle = (state) => state.hasOwnProperty(TITLE); | ||
|
||
export const hasContent = (state) => state.hasOwnProperty(CONTENT); | ||
|
||
export const hasDocuments = (state) => state.hasOwnProperty(DOCUMENTS); | ||
|
||
//type | ||
export const isValidArray = (arr) => arr && Array.isArray(arr); |
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.
p6) 제가 생각하기에 현재 error 파일 속 함수들은 전부 유효성 검사를 수행하는 것으로 보이는데, 이와 같은 경우 validation과 같은 file name이 더 적합하지 않을까.. 조심스레 의견 남겨봅니다 :)
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.
정말 바쁘셨을텐데 대단하신 것 같습니다..!
배포된 사이트가 혹시 여기 맞나요? -> https://kal-mh.github.io/notion.github.io/
디자인이 진심으로 훌륭해서 보기 편하네요. 마라탕은 안 먹어봐서 맛있는지 모르겠습니다 ㅎㅎㅎ
그리고 또 코드 보면서 느낀 점으로는, 전체적으로 상수화를 잘 하신 것 같아 저의 귀차니즘을 또 한 번 자책하게 되는 계기가 되었습니다! ㅋㅋㅋ
5주 동안 저의 옹알이 들으면서 참으신다고 고생 많으셨습니다. 묵묵히 역할도 잘 수행하시는 모습도 멋있었고용! 다음 오프라인 때는 꼭 봐요!👍
height: 100%; | ||
border: none; | ||
resize: 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.
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.
아니, API 숨기는 방법을 아직 찾지 못해서 차마 PR에 적어놓지 못했는데 언제 보셨답니까 ㅋㅋ 그래서 말인데, 배포하실 때, 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.
아니 어떻게 주인이 공개하기도 전에 아셨답니까 ㅋㅋㅋ 정말 CSS를 깔끔하게 작성하셨네요!!
documentPage.setState(documentsList); | ||
if (pathname === "/") { | ||
editPage.setState(DEFAULT_STATE); | ||
} else if (pathname.indexOf(`${SLASH_DOCUMENTS}`) === 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.
오홋 /documents로 시작되는 경우에만 렌더링되게 하신 거군요! 나중에 확장성을 고려한다면 좋은 조건문이라고 생각합니다!
}); | ||
|
||
new DocumentFooter({ $target: $page }); | ||
|
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.
사소할 수 있지만 이렇게 DocumentHeader, Documents, DocumentFooter가 순서대로 와서 마음이 편안하네요. ㅎㅎㅎ
routeRemoveDocument({ id, parentId }); | ||
} | ||
}); | ||
} |
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에서 한번에 처리하도록 만든거군요!
${createNewHTML(innerTags.substring(0, innerTags.length - 5), { | ||
id: DEFAULT_ID, | ||
title: DEFAULT_TITLE, | ||
padding: parseInt(paddingLeft.substring(0, paddingLeft.length - 2)) + PADDING_INCREMENT, |
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.
P4 : depth가 깊어질수록 padding 값을 증가시키는 방식 같은데 제가 이해한게 맞을까요?
저도 구현하면서 알게 된 것인데 ul 태그 안에 ul 태그가 들어가면! 첫 번째 ul 태그의 padding이 40px이라면, 두 번째 ul 태그의 padding은 그 앞의 40px과 합쳐져 80px이 됩니다. padding-inline-start라는 ul 태그의 기본 스타일 덕분인 것 같더라구요. 그래서 depth가 깊어져도 알아서 padding이 증가하게 되는 효과가 되더라구요. 이를 통해서 코드를 조금 더 간결하게 만들 수 있었던 것 같아요.
간단하게 예제를 만들어봤는데 이렇게 단순하게 HTML로만으로도 깊이를 더할 수 있네요! 뀨!
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.
좋은 정보 감사해요:) Notion을 뜯어보다가 모두 div로 되어 있는 것을 보고 저도 그쪽으로 방향을 잡았는데, 더 영리한 방법이 있었군요 음..
const documentBlockList = e.target.document.body.querySelectorAll(`.${DOCUMENT_BLOCK}`); | ||
let currentDocumentBlock = null; | ||
|
||
documentBlockList.forEach((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.
의도에 맞게 map이 아니라 forEach를 잘 사용하지 않았나라는 생각이 드네요!
}, | ||
}) | ||
); | ||
}; |
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.
P5 : custom route event를 발생시키기 위한 코드들인 것 같은데 맞을까요? 밖에서 인자를 던지는 식으로 하고 여기선 그걸 받아서 처리하는 식으로 하면 중복을 조금 더 줄일 수 있겠다라는 생각이 들었어요!
export const dispatchRouteEvent = (data, type) => {
const { id, parentId, title, content } = data;
window.dispatchEvent(
new CustomEvent(type, {
detail: {
id,
parentId,
title,
content,
},
})
);
};
id, parentId, 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.
미현님!
바쁘셨을텐데 과제하시느라 수고많으셨습니다.
승준님이 '대신 올리신' 배포 사이트의 깔끔한 UI를 보고 깜짝 놀랐습니다 🤯
왜 배포하셨는데 자랑 안하셨나요ㅜㅠ...
CustomEvent를 이용해 데이터 저장, 수정, 삭제하셨는데 이렇게도 활용될 수 있구나를 알게 되었습니다~ 전 커스텀 이벤트에 대해 더 공부해보아야겠네요...
어느덧 마지막 코드 리뷰인데 많이 아쉽네요ㅜㅠ
5주 동안 수고하셨고 같이 공부할 수 있어서 너무나 좋았습니다! 🥰
내년 1월에 뵈어요!
height: 100%; | ||
border: none; | ||
resize: 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.
아니 어떻게 주인이 공개하기도 전에 아셨답니까 ㅋㅋㅋ 정말 CSS를 깔끔하게 작성하셨네요!!
this.render = () => { | ||
$footer.innerHTML = ` | ||
<div class="${DOCUMENT_BLOCK_INNER}" style="padding: 2px 10px 2px ${DEFAULT_PADDING}px"> | ||
<div class="${NEW_BTN}"></div> | ||
<div class="${TITLE}"> ${CONTENT} </div> | ||
</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.
클래스 명까지 상수로 관리하시다니!!! 더 보기 좋은 것 같아요! 👀
export const createDocumentSection = (parentDocument, padding, displayMap) => { | ||
const { id, title, documents } = parentDocument; | ||
|
||
if (id && title !== undefined) { | ||
return ` | ||
<div class="${DOCUMENT_SECTION}"> | ||
${createDocumentBlock(id, title, padding)} | ||
${ | ||
documents.length | ||
? ` | ||
<div data-id="${id}" class="${DOCUMENT_LIST_BLOCK}" | ||
style="display:${displayMap[id] ? "block" : "none"}"> | ||
${documents | ||
.map((document) => createDocumentSection(document, padding + PADDING_INCREMENT, displayMap)) | ||
.join("")} | ||
</div> | ||
` | ||
: "" | ||
} | ||
</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.
P4
: display를 이용해서 열린 문서들을 보이거나 숨기도록 하셨군요!!
고런데 display: none; 적용했더라도 DOM 트리에 추가되는 걸로 알고 있습니다...(만약 아니라면 말씀해주세요!)
displayMap을 이용해서 해당 문서의 id가 true일 경우에만 랜더링되도록 하는 것은 어떨까요???
if (id && title !== undefined) {
return `
<div class="${DOCUMENT_SECTION}">
${createDocumentBlock(id, title, padding)}
${
documents.length
? `
<div data-id="${id}" class="${DOCUMENT_LIST_BLOCK}">
${displayMap[id] ? documents
.map((document) =>
createDocumentSection(
document,
padding + PADDING_INCREMENT,
displayMap
)
)
.join("") : ""}
</div>
`
: ""
}
</div>
`;
이렇게 고치면 되려나요? 헤더 x-username를 api_constant.js 파일에 같이 숨기셔서 실행해보지는 못해서 맞는지는 잘 모르겠네요!😝
window.addEventListener(EVENT_ROUTE_CREATE, async (e) => { | ||
const { id } = e.detail; | ||
|
||
const createNewDocument = await request(`${SLASH_DOCUMENTS}`, { | ||
method: "POST", | ||
body: JSON.stringify({ | ||
title: DEFAULT_TITLE, | ||
content: "", | ||
parent: id, | ||
}), | ||
}); | ||
console.log(createNewDocument); | ||
|
||
routePush(`${SLASH_DOCUMENTS}/${createNewDocument.id}`, 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.
customEvent를 이용해서 request를 실행하는군요!? 이렇게도 활용할 수 있네요~ 저는 Sidebar 컴포넌트 내부에서 e데이터 가져오고 수정하고 삭제하도록 작성했는데 router.js에 코드 분리되어 더 보기 좋다는 생각이 듭니다. 😊
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.
안녕하세요, 미현님! 리뷰가 늦어서 죄송합니다아..ㅠ.ㅠ 늦었지만 마지막 리뷰 남겨봤습니다..🥺
다른 분들이 꼼꼼히 작성해주셔서 열심히 따봉만 눌렀네요..ㅎㅎ
custom event 사용하시면서 컴포넌트 간 결합도는 더 낮아지지 않았을까 조심스럽게 생각해봅니다! 👍🏻
정신없는 와중에 이렇게 멋지게 구현하시다니 대단하셔요!! 다음 오프라인 만남 때는 꼭 뵈요 미현님!! >_<❤️
수고하셨습니다! :-D
const removeAllDocument = async (document) => { | ||
for (const subDocument of document.documents) { | ||
removeAllDocument(subDocument); | ||
} | ||
|
||
await request(`${SLASH_DOCUMENTS}/${document.id}`, { | ||
method: "DELETE", | ||
}); | ||
}; |
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들까지 모두 삭제되도록 구현하셨네요!!
저도 이게 더 자연스러워 보이는 것 같습니다! 👍🏻
export const setHeaderChange = ({ id, title }) => { | ||
window.dispatchEvent( | ||
new CustomEvent(EVENT_HEADER_CHANGE, { | ||
detail: { | ||
id, | ||
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.
전 라우팅 이외에 CustomEvent 적용을 못해봤는데ㅠ.ㅠ 미현님은 적용시키셨군요! 👍🏻 멋지십니다!
- DeleteButton.js, documentUtil.js js를 통해서 style변화시키는 부분 수정 -> className을 추가 - init()함수 설정 - 조건문 간략화(this.state.id === DISABLE_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.
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 이슈
📌 과제 설명
바닐라 JS만을 이용해 노션을 클로닝합니다.
👩💻 요구 사항과 구현 내용
Components
App
/
: 루트 url일 경우, Root Page를 보여줍니다. 편집은 할 수 없습니다./documents/:id
: 특정 document의 title과 content를 보여줍니다.DocumentPage
전체 화면 상에서 사이드바에 해당하는 component입니다.
DocumentHeader
워크 스페이스 문구와 새 문서 생성하는 버튼을 가지고 있습니다.
Documents
전체 문서 목록을 보여줍니다.
재귀함수를 통해 하위 문서까지 포함하여 목록을 생성합니다.
document-section 영역은 부모 문서에 해당하는 document-block과 하위 문서를 감싸는 document-list-block을 포함하고 있습니다.
document-block은 세 가지 기능의 버튼을 가지고 있습니다.
display-btn
: 하위 목록 펼침. 펼침 여부는 localStorage에 저장합니다.remove-btn
: 문서 삭제new-btn
: 새 문서 생성새로운 문서를 생성할 때는 낙관적 업데이트를 통해 먼저 태그 요소를 보여주고, API요청을 보냅니다. (documentUtil.js)
DocumentFooter
사이드바의 footer로써 새 문서 생성하는 버튼이 있습니다.
EditorPage
편집 기능을 갖춘 화면을 렌더링합니다.
autoSave()
를 통해 서버에 자동저장합니다.Topbar
delete
버튼을 포함하고 있습니다.DeleteBtn
Header
문서의 title을 작성하는 공간입니다.
input
태그를 이용하여 입력이 들어오는 이벤트가 발생할 때마다onEditing()
을 호출합니다.setHeaderChange()
: 사이드바에서 현재 선택된 문서의 title을 동시에 같이 수정합니다.Editor
문서의 content를 작성하는 공간입니다.
textarea
태그를 이용하여 입력이 들어오는 이벤트가 발생할 대마다onEditing()
을 호출합니다.router.js
각 태그에서 발생하는 이벤트에 대해서 이벤트 핸들러를 작성한 모듈입니다.
EVENT_ROUTE_PUSH
: 특정 문서를 선택EVENT_ROUTE_REMOVE
: 특정 문서를 삭제EVENT_ROUTE_CREATE
: 새 문서 생성EVENT_ROUTE_PUT
: 문서 수정EVENT_HEADER_CHANGE
: 문서 title 수정 시, 사이드바 문서 제목 동기화✅ 피드백 반영사항
-
PROPERTISE
상수 집합 객체 error.js파일 안으로 이동- DocumetHeader, DocumentFooter content 상수값 각각 파일 안으로 이동
[, , documentId] = pathname.split("/")
리팩토링✅ PR 포인트 & 궁금한 점
과제를 타이트하게 진행함에 따라서, 저번 week3 VanillaJS 프로젝트에서의 피드백을 온전히 반영하지 못한 부분이 있습니다 (ex) isInit flag => init()). 해당 부분은 코드리뷰 반영 기간 때, 수정될 예정입니다
각각의 컴포넌트들끼리의 응집성, 결합성이 어떠한 지 피드백을 받고 싶습니다.