-
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 #30
base: 3/#4_yangsangwoo
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.
안녕하세요 상우님! 같은 유리팀 유지영입니다 😊
첫 번째 개인 프로젝트 수고하셨습니다! 상우님 코드를 보면서 이래서 설계가 중요하다는 걸 다시 한 번 느꼈어요! 자주 사용되는 함수들을 다 따로 만드신 부분에서 많이 배웠습니다!
수고하셨습니다 👍
@@ -0,0 +1,3 @@ | |||
export const APU_END_POINT = 'https://kdt-frontend.programmers.co.kr/documents'; | |||
|
|||
export const USER = 'yangsangwoo'; |
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 주소가 노출되지 않도록 .gitignore 파일에 추가하는 게 좋을 것 같아요! :)
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.
이부분은 commit 수정이 필요할 것 같습니다!
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 @@ | |||
export const $ = ({selector}) => document.querySelector(selector); |
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 (classList.contains('remove')) { | ||
if (confirm('삭제하시겠습니까?')) { |
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/:id에 해당한다면 해당 id의 문서를 랜더링 | ||
if (pathname.includes('/documents/')) { | ||
const documentId = pathname.split('/').at(-1); |
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.
at()메서드는 처음보네요!! 이렇게 사용하는 메서드라니 처음 알았습니다 ㅎㅎ 배워갑니다 ! :)
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://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Array/at
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.
맞습니다~! 호환성이 떨어져서 length-1을 이용하는게 더 안전할것 같아요~!
질문에 대한 정확한 답은 아니지만, 저는 이러한 바닐라 과제를 할때 리액트의 고유 형태를 너무 쫓지 않았으면 합니다. 리액트의 state 관리과 자체 구조는 바닐라로 간단하게 구현하기에는 한계가 있고 이를 1주짜리 과제에 적용하기는 스펙오버라고 생각해요. |
"start": "parcel index.html", | ||
"build": "parcel build index.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.
parcel을 사용하는게 꼭 틀린건 아니지만...현재로써는 거의 사양되고 있는 번들러여서, 웹팩이나 롤업에 대한 학습이 동반되면 좋을 것 같네요
const doc = await LIST_API.editDocument({ id: docId, content }); | ||
const $doc = $({ selector: `[data-index="${docId}"] > span` }); | ||
|
||
$doc.innerText = doc.title; | ||
removeDocument(`document-${docId}`); |
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.
이렇게 select를 하는 구조보다 더 나은 방법은 없을까요?
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로 되어있는데 해당 부분을 변경할 수 있는 로직으로 사용하면 좋을것 같다는 생각이 듭니다~!
const makeParam = () => { | ||
switch (method) { | ||
case 'GET': | ||
if (Boolean(id)) return { path: `/${id}` }; | ||
return { path: '' }; | ||
case 'POST': | ||
return { path: '', content }; | ||
case 'PUT': | ||
return { path: `/${id}`, content }; | ||
case 'DELETE': | ||
return { path: `/${id}` }; | ||
default: | ||
throw new Error('잘못된 method 입니다.'); | ||
} | ||
} | ||
|
||
try { | ||
const response = await API[method](makeParam()); |
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.
내부에서 함수로 처리하지 않아도 되었을 것 같아요
import { APU_END_POINT, USER } from '../../dummyEnv.js'; | ||
|
||
const API_ROOT = ({ path }) => `${APU_END_POINT}${path}`; | ||
|
||
const returnOptions = ({ method, content }) => { | ||
return { | ||
method, | ||
headers: { | ||
'x-username': USER, | ||
[method === 'POST' || method === 'PUT' ? 'Content-Type' : 'Accept']: | ||
'application/json', | ||
}, | ||
[method === 'POST' || method === 'PUT' ? 'body' : 'params']: | ||
JSON.stringify(content), | ||
}; | ||
}; | ||
|
||
const useFetch = async ({ path, method, content = '' }) => { | ||
try { | ||
const options = returnOptions({ method, content }); | ||
const response = await fetch(API_ROOT({ path }), options); | ||
|
||
if (!response || !response.ok) { | ||
throw Error(response.status); | ||
} | ||
|
||
return await response.json(); | ||
} catch (e) { | ||
alert(e.message); | ||
console.error(e.message); | ||
} | ||
}; | ||
|
||
const API = { | ||
GET: async ({ path }) => await useFetch({ path, method: 'GET' }), | ||
|
||
POST: async ({ path, content }) => | ||
await useFetch({ path, method: 'POST', content }), | ||
|
||
PUT: async ({ path, content }) => | ||
await useFetch({ path, method: 'PUT', content }), | ||
|
||
DELETE: async ({ path }) => await useFetch({ path, method: 'DELETE' }), | ||
}; | ||
|
||
export default 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.
api 관련 함수들을 정리해보면 좋을 것 같습니다!
우선 리액트의 hook이 아닌데 useXXX라는 네이밍보다 자연스러운 네이밍으로 변경하면 좋을 것 같습니다
그리고 함수를 너무 분리할 필요는 없는 것 같아요
LIST_API.getAllDocuments에 오기까지 많은 유틸함수를 타고 있는데, 이렇게 분리해서 작업하지 않아도 될 것 같습니다
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.
너무 분리한것 같아서 리펙토링에서는 computed properties를 이용해 봐야겠습니다!
async onGetAllDocument() { | ||
return await LIST_API.getAllDocuments(); | ||
} |
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.
getAllDocuments가 비동기 함수라고 해서 await를 붙이지 않아도 됩니다ㅎㅎ
오히려 매번 await 하고 반환할때 성능이 감소한다는 벤치마크 결과도 있어요
"useTabs": true, | ||
"tabWidth": 2, |
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.
개인 취향이긴 하지만, tab은 안쓰는게 좋은 것 같네요! (화면을 보시다시피, 인덴트가 굉장히 길어집니다..)
|
||
const rootParent = summary.closest('details').parentNode?.id; | ||
|
||
alert('삭제되었습니다.'); |
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.
confirm도 system alert인데, alert이 또 뜨면 사용자 경험을 해칠 수 있을 것 같습니다ㅎㅎ
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 generateHTML = () => { | ||
return `<img class="greeting-img" src="${img}" alt="greeting-img"/>`; | ||
}; | ||
|
||
this.render = () => { | ||
$target.innerHTML = generateHTML(); |
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.
generateHTML 함수를 안 만들고 바로 생성해도 되지 않았을까요?
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 (!isRendered) { | ||
$target.innerHTML = generateHTML(); | ||
isRendered = true; | ||
} |
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.
현재 render가 되고 있는지를 확인하는 플래그였는데 큰 의미가 없을것 같네요..! 감사합니다!
|
||
this.render = ({ state }) => { | ||
$target.innerHTML = generateHTML(); | ||
refelectDocumentValue({ nextState: 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.
refelect
오타같습니다~!
return ` | ||
<button class="sub-document-item" data-index="${id}"> | ||
<img class="doc-img" src="${img}" altx="doc-img"/> | ||
${title ? 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.
title || '제목없음'
으로 처리할 수 있을 것 같습니다
altx="doc-img"
오타같습니다~
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도 놓치지 않는 섬세함까지.. 많이 배워갑니다 👍
.option-btn { | ||
position: relative; | ||
z-index: 999; | ||
display: none; | ||
margin-top: 2px; | ||
background: rgba(55, 53, 47, 0.08); | ||
border: 1px solid rgba(55, 53, 47, 0.16); | ||
padding: 2px; | ||
margin-bottom: 2px; | ||
width: 10px; | ||
height: 10px; | ||
border-radius: 2px; | ||
transition: 0.3s; | ||
} |
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.
리스트에 hover 했을 때 버튼이 보이게 되면 하위 리스트가 아래로 조금씩 밀리더라고요!
버튼 크기를 수정하거나 패딩 값을 조절해서 밀리지 않게 하는 게 좋을 것 같아요 :)
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 rootParent = summary.closest('details').parentNode?.id; | ||
|
||
alert('삭제되었습니다.'); |
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,3 @@ | |||
export const APU_END_POINT = 'https://kdt-frontend.programmers.co.kr/documents'; | |||
|
|||
export const USER = 'yangsangwoo'; |
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.
이부분은 commit 수정이 필요할 것 같습니다!
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 compareStateWithStoredState = async ({ nextState, state }) => { | ||
const { id } = nextState; | ||
const key = `document-${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.
함수나 변수명은 항상 짧고 간결하게만 작성해왔는데, 길지만 명확한 이름으로 이해가 잘되는 것 같습니다
배워갑니다 ~ :)
const useFetch = async ({ path, method, content = '' }) => { | ||
try { | ||
const options = returnOptions({ method, content }); | ||
const response = await fetch(API_ROOT({ path }), options); | ||
|
||
if (!response || !response.ok) { | ||
throw Error(response.status); | ||
} | ||
|
||
return await response.json(); | ||
} catch (e) { | ||
alert(e.message); | ||
console.error(e.message); | ||
} | ||
}; | ||
|
||
const API = { | ||
GET: async ({ path }) => await useFetch({ path, method: 'GET' }), | ||
|
||
POST: async ({ path, content }) => | ||
await useFetch({ path, method: 'POST', content }), | ||
|
||
PUT: async ({ path, content }) => | ||
await useFetch({ path, method: 'PUT', content }), | ||
|
||
DELETE: async ({ path }) => await useFetch({ path, 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.
오옹 메소드를 따로 빼서 작성하니까 훨씬 깔끔하네요 :)
export const createDOMElement = ({ tagName, attrs = null }) => { | ||
const element = document.createElement(tagName); | ||
attrs && | ||
attrs.forEach(({ attr, value }) => { | ||
attr !== 'textContent' | ||
? element.setAttribute(attr, value) | ||
: (element.textContent = value); | ||
}); | ||
return 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.
파일명 오타인 것 같습니다~ ㅎㅎ
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 debounce = (func, wait) => { | ||
let timerId = null; | ||
return (...args) => { | ||
if (timerId) clearTimeout(timerId); | ||
|
||
timerId = setTimeout(() => { | ||
func(...args); | ||
}, wait); | ||
} | ||
} |
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.
util에 해당되는 함수들을 최대한 따로 빼서 import하는 방식으로 구현하니까 너무 좋은 것 같아요... !!
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를 모듈처럼 분리하신 부분이나 App을 클래스로 구현하신 부분이 인상적이었습니다~
다만 멘토님 말씀처럼 util 함수가 너무 잘게 쪼개진 느낌을 받긴 했습니다!
util함수로 구분하는 이유에는 다소 복잡한 로직을 반복적으로 실행하지 않기 위한 성격이 강할 텐데, querySelector 같은 부분이 굳이 분리될 필요가 있을까 싶었어요! 더군다나 $로 querySelector의 메소드 이름을 변경해주셨는데 제이쿼리같은 걸 잘 모르는 사람(저같은 사람..)에겐 변수명에도 $가 붙어있다보니 다소 혼란스러웠던 것 같습니다..
물론 이런건 코드 스타일의 문제라 굳이 바꾸실 필요는 없을 것 같아요! 그냥 '이런 느낌을 받으셨구나~'하고 지나가셔도 좋습니다.
전체 앱의 각 구성요소와 디테일한 부분을 신경 써서 나누신 부분은 정말 배우고 싶은 부분이었어요! 덕분에 많은 깨달음을 얻고 갑니다~
} | ||
|
||
const main = new Main(); | ||
main.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.
깨알 피드백이지만 init을 Main 내부에서 실행해버리면 new하면서 자동으로 실행되니까 따로 실행 안해줘도 될 것 같아요! 근데 이건 코드 스타일의 차이인 것 같기도 해서 그냥 의견만 남겨봅니다
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.
오호 그렇네요~! 감사합니다!
<meta charset="UTF-8" /> | ||
<link rel="icon" href="./src/public/images/fabicon.ico" /> | ||
<link rel="stylesheet" href="./src/styles/index.css" /> | ||
<script src="./src/Main.js" type="module"></script> |
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.
module 스크립트를 head에 넣으면 script를 먼저 실행하고 html 파싱을 이어가는 걸로 알고 있는데, main이 DOM에 없는 상태에서도 동작이 되는 이유가 궁금해요!
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.
module은 기본적으로 defer를 지원하기 때문에 html 파싱 이후에 script가 실행되게 해줍니다~!
@import './reset.css'; | ||
@import './SideBar/SideBar.css'; | ||
@import './DocumentPage/DocumentPage.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.
오오 이렇게 css를 모듈처럼 쓸 수 있군요! 잘 배우고 갑니다!!
<input type="button" class="option-btn remove" /> | ||
<input type="button" class="option-btn 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.
input 으로 button을 하신 이유가 있을까요? 개인적으로는 버튼의 콘텐츠로 이미지 추가 등을 하기엔 <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.
사실 form안에 button 태그는 기본적으로 submit을 발생하기에.. e.preventEvent를 해줘야하는 귀찮음이 있어서 input으로 했습니다 ㅎㅎㅎ 사실 시멘틱하게 생각하면 button으로 만드는게 올바르죠~
4주차 Notion 클로닝 과제
데브코스 기술 과제 repository fork
바닐라 JS만을 이용해 노션을 클로닝합니다.
기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.
📥 Download
npm
👨👨👦 Contributor
양상우 ( Front-End )
🔍 Features
📚 Assignment
기본 요구사항
보너스 요구사항
🛠 구현 설명
1. 설계
2. 컴포넌트 구조
3. 컨벤션
commit 컨벤션
🤔 궁금한점
리스너가 많아질 경우 어떻게 처리를 하면 좋을까요?