-
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 #13
base: 3/#4_shinseungjun
Are you sure you want to change the base?
Conversation
- storage 메서드 구현 - Sidebar 컴포넌트 작성 - DocumentList 컴포넌트 작성
- 위 3 기능 모듈화 필요 - Button svg 파일 추가 - App으로부터가 아니라 Sidebar로부터 rootDocuments state가 내려오도록 변경
- createElement 시 attributes도 설정되도록 모듈화 - Sidebar와 Post의 Header가 각각 다르게 나올 수 있도록 변경
… 변경되도록 구현 - PostEditor 화면 렌더링 시 title에 focus 되는 기능 구현
- push, replace만으로는 어떤 행동을 하는지 알기 어렵다고 생각하여 이름 변경
- 사용하지 않는 코드 삭제 - currentPath 변경 모듈화
- templates 코드 개선
- Sidebar의 Header 또한 클릭 시 배경색이 달라지도록 구현
- debounce time 250ms로 변경
- routing할 때 selected가 변경되도록 개선
- index.html title 변경
- selected className 조작하는 코드 모듈화 - /utils/helpers 폴더 생성해 애매한 함수들을 모두 담음.
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 = () => { | ||
$notFound.innerHTML = ` | ||
<h1>문서를 선택해주세요.</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.
p6) Not Found 페이지에서 문서를 선택하라는 문구는 조금 어울리지 않다는 생각이 드는 것 같습니다!! ㅎㅎ
@@ -0,0 +1,24 @@ | |||
const createElement = ({ |
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.
저도 모든 파일에 element 만들고 클래스 넣고 하는 과정이 반복됐다고 생각했는데 이렇게 따로 빼면 됐네요.. ㄷㄷ 써먹겠습니다!
const { subDocumentList } = this.state; | ||
const { currentPath } = this.state; | ||
|
||
$div.innerHTML = ` | ||
${subDocumentList | ||
.map( | ||
({ id, title }) => ` | ||
<div class='document-item' title='${title}' data-current-path='${currentPath} > ${title}' data-id='${id}'> | ||
<div class='document-item__title'>${title}</div> | ||
</div> | ||
`, | ||
) | ||
.join('')} | ||
`; | ||
}; |
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) 승준님 현재 배포하신 페이지에서 하위 페이지를 추가해도 subDocumentList가 업데이트 되지 않는 것 같은데 한 번 확인해 보셔야 될 것 같아요! ㅎㅎ
this.render = () => { | ||
const openedDocumentItems = getItem(OPENED_DOCUMENT_ITEMS, []); | ||
$documentList.innerHTML = ` | ||
${this.state |
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.
엄청난 indent..
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 ( | ||
[ | ||
'document-list', | ||
'document-list__root', | ||
'document-item-container', | ||
'no-sub-document', | ||
].includes(target.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.
p6)
element.classList.contains를 사용해서 element의 클래스를 체크할 수 있습니다!
element.classList.contains('document-list', ...)
제가 알기로는 하나라도 일치하지 않으면 false를 반환합니다!
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.
승준님처럼 표현할 수도 있군요, 새로운 방법 배워갑니다:)
src/styles/index.css
Outdated
} | ||
|
||
.document-item-container { | ||
/* cursor: pointer; */ |
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.
꺄악!
src/utils/helpers/validator.js
Outdated
const isValidElement = () => {}; | ||
|
||
export { isValidElement }; |
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) 지워도 될 것 같은 파일이네요!
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.
승준님, 한 달 동안 수고많으셨습니다:)
helpers로 모듈들을 정리해 놓은 것은 처음 보는 것 같은데 utils 안에서도 다양하게 나눠놓은 것을 보고 PR에서도 언급했었던 것처럼 효율적으로 나누기 위해 노력하셨구나 알 수 있었습니다.
저도 스스로는 설계를 하고 코드를 작성하는 루틴을 가지고 있다고 생각했었는데, 이번 프로젝트를 진행하면서 순간순간 떠오르는 기능들을 빨리 구현해보고 싶은 마음에 휘뚜르마뚜르 일단 쭉 작성해보고 리팩토링을 제출 전까지 거듭해서 했었던 기억이 나네요.
디자인 패턴이나, 아키텍처를 효율적으로 습득할 수 있는 법은 저도 너무 알고 싶어요ㅎ
디자인적인 면에서 한 가지 리뷰사항이 있습니다.
하위 문서를 계속 생성해나갈 때, "하위 페이지가 존재하지 않습니다" 텍스트가 overflow되어서 바로 밑 문서의 title 영역을 침범하네요. text-overflow를 지정해주시면 되지 않을까 싶습니다
수고 많으셨어요:)
); | ||
}, | ||
onClickDocumentItemDeleteButton: async id => { | ||
if (!confirm('해당 문서를 삭제하시겠습니까?')) 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.
삭제하기 전에 한 번 더 물어보는 것도 좋은 거 같네요!
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.
감사합니다~
그리고 덕분에 하위 페이지가 존재하지 않습니다 이거 overflow 처리했어요!
if ( | ||
[ | ||
'document-list', | ||
'document-list__root', | ||
'document-item-container', | ||
'no-sub-document', | ||
].includes(target.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.
승준님처럼 표현할 수도 있군요, 새로운 방법 배워갑니다:)
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.
승준님!
이번에도 코드를 꼼꼼하게 작성하셔서 보기 편했던 것 같아요! 😎
진정한 I의 광기를 보았나요..? ㅋㅋㅋ
마지막 코드 리뷰라 많이 아쉽습니다...
항상 새벽까지 공부하시고 대단하십니다...👍 덩달아 저도 많이 공부했습니다~
5주 동안 수고하셨고 같이 공부할 수 있어서 너무나 좋았습니다! 🥰
내년 1월에 뵈어요!
const $article = createElement({ | ||
element: 'article', | ||
$target, | ||
}); |
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.focus = () => { | ||
postEditor.focus(); | ||
}; |
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 = () => { | ||
const openedDocumentItems = getItem(OPENED_DOCUMENT_ITEMS, []); | ||
$documentList.innerHTML = ` | ||
${this.state |
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; | ||
documentList.setState([...this.state]); | ||
}; |
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
: documentList.setState(this.state);
라고 작성해도 되지 않을까요?? 아니면 다른 이유가 있으실까요?? 궁금해요 👀
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.
맞아요.. 그대로 작성하면 될 것 같습니다! React에서 setState해줄 때 재할당하던 것을 생각해서 여기에서도 이렇게 해주었었습니다. 근데 굳이 재할당하지 않아도 setState로 인해 render까지 제대로 작용할 거니까 이 Vanilla JS 프로젝트에서는 하지 않아도 되겠네요!
const TRIGGERS = { | ||
heading1: { | ||
trigger: '# ', | ||
tagName: 'h1', | ||
offset: 2, | ||
}, | ||
heading2: { | ||
trigger: '## ', | ||
tagName: 'h2', | ||
offset: 3, | ||
}, | ||
heading3: { | ||
trigger: '### ', | ||
tagName: 'h3', | ||
offset: 4, | ||
}, | ||
}; |
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.
하드 코딩하지 않으려 노력하시는 모습...! 특히나 offset이 정의되어 있어서 리치 에디터 구현하기 편할 것 같아요!
const modifyStorage = { | ||
add: id => { | ||
const openedDocumentItemIds = getItem(OPENED_DOCUMENT_ITEMS, []); | ||
if (!openedDocumentItemIds.includes(id)) { | ||
setItem(OPENED_DOCUMENT_ITEMS, [...openedDocumentItemIds, id]); | ||
} | ||
}, | ||
delete: id => { | ||
const openedDocumentItemIds = getItem(OPENED_DOCUMENT_ITEMS, []); | ||
const removedOpenedDocumentItemIdIndex = openedDocumentItemIds.findIndex( | ||
openedDocumentItemId => openedDocumentItemId === id, | ||
); | ||
if (removedOpenedDocumentItemIdIndex !== -1) { | ||
openedDocumentItemIds.splice(removedOpenedDocumentItemIdIndex, 1); | ||
} | ||
setItem(OPENED_DOCUMENT_ITEMS, [...openedDocumentItemIds]); | ||
}, | ||
toggle: id => { | ||
const openedDocumentItemIds = getItem(OPENED_DOCUMENT_ITEMS, []); | ||
if (openedDocumentItemIds.includes(id)) { | ||
const removedOpenedDocumentItemIdIndex = openedDocumentItemIds.findIndex( | ||
openedDocumentItemId => openedDocumentItemId === id, | ||
); | ||
if (removedOpenedDocumentItemIdIndex !== -1) | ||
openedDocumentItemIds.splice(removedOpenedDocumentItemIdIndex, 1); | ||
setItem(OPENED_DOCUMENT_ITEMS, [...openedDocumentItemIds]); | ||
} else { | ||
setItem(OPENED_DOCUMENT_ITEMS, [...openedDocumentItemIds, 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.
저는 Sidebar에서 몽땅 작성했는데 저도 이렇게 작성해보아야겠어요~
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 이슈
|
||
const BASE_URL = 'https://kdt-frontend.programmers.co.kr/documents'; | ||
|
||
const request = async ({ url = '', options = {} }) => { |
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 인스턴스를 이렇게 함수로 감싸 돌려쓰는 것 너무 좋네요!
TMI
저는 보통 fetch 라이브러리를 쓰고 있어요.
ky나 axios 같은..?
const { pathname, search } = window.location; | ||
const [, , id] = pathname.split('/'); |
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;
현재 base path가 /some/some/:id
형태로 되어있는 것으로 보여지는데 맞을까요?
지금 당장은 큰 문제가 없겠지만 나중에 URL 구조가 변경되거나 할 때 이 부분을 놓칠 수도 있겠단 생각이 소소하게 들었어요!
개발자가 해당 변경사항을 잘 알 수있게 하는 방법 등을 고려해보셔도 좋을 것 같아요!
📌 과제 설명
바닐라 JS만을 이용해 노션을 클로닝합니다.
배포 링크
AWS S3 + CloudFront를 이용했습니다. https://metamong.site/
👩💻 요구 사항과 구현 내용
요구사항
보너스 요구사항
그 외 개선한 사항들
파일 구조
src
┣ assets
┃ ┣ images
┃ ┃ ┣ addButton.svg
┃ ┃ ┣ deleteButton.svg
┃ ┃ ┗ toggleButton.svg
┃ ┗ .empty
┣ components
┃ ┣ Post
┃ ┃ ┣ NotFound.js -> 선택한 문서가 없을 시 렌더링되는 컴포넌트
┃ ┃ ┣ PostComponent.js
┃ ┃ ┣ PostEditor.js
┃ ┃ ┗ SubDocumentList.js -> 선택한 문서의 하위 문서 링크를 렌더링하는 컴포넌트
┃ ┣ Sidebar
┃ ┃ ┣ DocumentList.js
┃ ┃ ┣ RootDocumentAddButton.js
┃ ┃ ┗ SidebarComponent.js
┃ ┗ shared -> 공통적으로 사용되는 컴포넌트
┃ ┃ ┗ Header.js
┣ styles
┃ ┣ index.css
┃ ┗ normalize.css
┣ utils
┃ ┣ api
┃ ┃ ┣ apis.js
┃ ┃ ┗ baseConfig.js
┃ ┣ constants
┃ ┃ ┣ errorMessages.js
┃ ┃ ┗ paragraphs.js
┃ ┗ helpers -> 추상화를 돕는 코드들
┃ ┃ ┣ changeBackgroundColorOfSelectedItem.js
┃ ┃ ┣ changeCurrentPath.js
┃ ┃ ┣ createElement.js
┃ ┃ ┣ debounce.js
┃ ┃ ┣ makeRichContent.js
┃ ┃ ┣ router.js
┃ ┃ ┣ storage.js
┃ ┃ ┣ templates.js
┃ ┃ ┗ validator.js
┣ App.js
┗ index.js
버그 및 앞으로 개선할 것들
했던 고민들과 어려웠던 점들
느낀 점
✅ PR 포인트 & 궁금한 점