-
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 #29
base: 3/#4_jeonseunghwan
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.
안녕하세요 승환님! 코드리뷰 하러 왔습니다 😆
이번 과제 생각할게 너무 많았던 것 같습니다 ㅠ 고생 많으셧어요!
혹시 차후 리팩토링을 하게 된다면 Api로 무조건 호출을 하는 것 보단
LocalStorage의 활용 혹은 꼭 필요한 상황에만 통신을 할 수 있게
리팩토링 해 보는것도 좋은 경험이 될 것 같습니다 🤗
추가적으로 가능하다면 디자인을 조금 개선해보는 것도 좋을 것 같아요
현제 어디에 어떤 문서가 붙어있는 지 알기 힘든 상태인 것 같습니다 😂
1, 2는 저도 이번에 궁금해서 오프님한태 질문한거라 생략하겠습니다.
3
현재 삭제 기능에서 정상적으로 작동하는데 404 에러가 나오고 있습니다.
url이 잘못된 문제 같은데 원인을 못찾고 있습니다.
제 생각으로는 api 호출 문제이면 삭제 기능이 작동을 하면 안되는걸로 알고 있는데 아니면 리다이렉트에서 발생한 문제일까요..?
=> 이미 삭제된 id를 다시 삭제하라는 통신을 보내서 API에서 해당 문서를 찾을 수 없어 발생한 에러 같습니다. 😁
4
렌더링 될 때마다 화면이 깜빡거리는 증상이 있습니다. 원인을 못찾고 있습니다.
=> 렌더링 시 this.render에 appendChild를 하게 되는데 이 때 dom 전체가 다시 그려지는 것 같습니다.
새로고침도 잘 안되는것 같아서 아마 router 문제인거 같기도 한데 오프님이 자세히 알려주실 것 같습니다. ☺
}); | ||
}; |
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)
window.addEventListener('popstate', () => {
onRoute();
});
를 추가한다면 뒤로가기에도 대응이 가능할 것 같아요! 😏
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.
오옷 좋네요 👍
div: editor, | ||
initialState: initialState, | ||
|
||
onChangeContent: async ({ id, content }) => { | ||
setTimeout(await putContentMethod(id, content), 2000); | ||
}, | ||
}); |
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) keyup 시 들어있는 벨류 값이 api 통신으로 들아가게 작성하신 것 같습니다. 제대로 작성이 안됬는지
타이핑하는 순간 바로 키가 누른 숫자만큼 통신을 일으키고 있습니다 😥
Debounce 혹은 Throttle을 적용시켜서 통신의 간격을 두는 법 혹은
LocalData에 저장 후 일정 주기마다 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.
좋습니다~ Debounce 들어만 봤는데 한번 공부해보고 적용 시켜보겠습니다 👍
@@ -0,0 +1,44 @@ | |||
import { createElement } from '../utils/dom.js'; | |||
import { isNew, isString } from '../utils/errorHandler.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.
p5) 👍
src/Navbar/Navbar.js
Outdated
const cur = rootDocument.shift(); | ||
|
||
if (parseInt(cur.id) === parseInt(id)) { | ||
console.log(cur); |
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) 최종 제출시 console.log는 빼 주시는게 좋아요 😆
src/Navbar/Navbar.js
Outdated
// 현재 하위 document 가 3개 이상이면 하위 document 값이 삭제가 안되고 있음 | ||
// 하위 document 2개씩만 삭제되는 현상 | ||
const handlerDelete = async () => { | ||
const currentDocument = findCurrentId(); | ||
|
||
const que = [currentDocument]; | ||
|
||
const queId = [currentDocument.id]; | ||
|
||
while (que.length > 0) { | ||
const current = que.shift(); | ||
current.documents.forEach((el) => { | ||
que.push(el); | ||
queId.push(el.id); | ||
}); | ||
|
||
for (let i of queId) { | ||
await deleteMethod(i); | ||
} | ||
} | ||
}; | ||
|
||
await handlerDelete(); | ||
route('/'); | ||
}, |
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.
음 아무래도 제 생각으로는 말씀하신 것처럼 404 에러가 나오는 원인이랑 중복된 문서가 나오는게 api를 다시 호출하는게 원인 같습니다.
디버깅 돌려보니 que가 안비워져서 계속 실행되고 있었습니다.
que초기화 해주니 404 에러랑 중복 호출이 사라졌네요 👍 감사합니다 ㅎㅎ
src/Navbar/NavbarDocumentList.js
Outdated
if (targetContains(e, 'fa-minus-square')) { | ||
onDelete(postId); | ||
} else if (targetContains(e, 'fa-plus-square')) { | ||
onAdd(postId); | ||
} else { | ||
// refactor 해야함 | ||
route(`${documentsUrl}/${postId}`); |
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) 리팩토링 하게 된다면 else if 대신 if만으로 줄이는 방법을 고려하는것도 좋아 보여요! 😏
@zerosial |
"devDependencies": { | ||
"prettier": "2.7.1" | ||
}, | ||
"prettier": { |
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/style/index.css
Outdated
@@ -0,0 +1,138 @@ | |||
@import 'ui/index.css'; | |||
|
|||
/* Reset Browser */ |
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 module을 사용하는거라면 reset도 분리해서 import하는게 좋을거같아요 ㅎㅎ
src/utils/dom.js
Outdated
@@ -0,0 +1,5 @@ | |||
export const createElement=(e)=>document.createElement(e); | |||
export const $ =(e)=>document.querySelector(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.
음 이렇게 만들어서 사용하는것도 나쁘지는 않지만...
혹시 수화님의 의도가 아니게 jquery가 포함되어있을 경우 네임스페이스가 겹치게 되서 뭔가 의도와 다르게 될 수도 있습니다 ㅎㅎ
처음 코드를 접하는 분들은 jQuery를 사용하는 구나 라고 착각할수도 있구요!
오해의 여지는 줄일수 있다면 줄이는게 좋아보입니다.
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.
엇 이거는 제 습관이 된 것 같습니다.
처음 공부했을때부터 이렇게 사용을 해서 저도 모르게 quereySelector 는 $표현을 해버렸습니다.
이부분은 제가 다른 표현을 사용하거나 고치도록 하겠습니다 👍
src/utils/dom.js
Outdated
@@ -0,0 +1,5 @@ | |||
export const createElement=(e)=>document.createElement(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.
e면 event를 주로 의미하는데 createElement에 들어가는 타입과 맞지 않지 않나요
src/utils/dom.js
Outdated
export const targetClosest=(e,str)=>e.target.closest(str); | ||
export const targetContains=(e,str)=>e.target.classList.contains(str); |
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.
메서드 명을 좀 더 명확하게 하면 좋을것 같아요 ㅎㅎ
아래 메서드를 예시로 들면 isTargetContainsClassName
이런식으로요.
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/errorHandler.js
Outdated
export const isNew = (target) => { | ||
if (!target) { | ||
throw new Error(ERROR_MESSAGE.NEW); | ||
} | ||
}; | ||
|
||
export const isString = (title) => { | ||
if (!title instanceof String) { | ||
throw new Error(ERROR_MESSAGE.STRING); | ||
} | ||
}; | ||
|
||
export const isObject = (post) => { | ||
if (!post instanceof Object) { | ||
throw new Error(ERROR_MESSAGE.OBJECT); | ||
} | ||
}; |
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.
is가 붙은 함수들은 반환형이 boolean 형태인것으로 암묵적으로 사용하고 있습니다. 현재의 역할을 해야한다면 이름이 바뀌어야 할것같아요
import { request } from './api.js'; | ||
import { documentsUrl } from './util.js'; | ||
|
||
export const putTitleMethod = (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.
Method 접미사는 제거해도 좋을것 같아여 ㅎㅎ
src/Navbar/NavbarDocumentList.js
Outdated
if (targetContains(e, 'fa-minus-square')) { | ||
onDelete(postId); | ||
} else if (targetContains(e, 'fa-plus-square')) { |
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/Navbar/NavbarDocumentList.js
Outdated
this.render(); | ||
}; | ||
|
||
this.render = () => { |
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 바인딩을 굳이 하지 않아도 괜찮을것 같아요
this.state = nextState; | ||
const { title } = this.state; | ||
this.render(); | ||
editorTitle.querySelector('.title').value = 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 && 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.
zzzzㅋㅋ 다른 분들 코드보고 만들다가 삼항연산자 대신에 사용했는데 없어도 되겠네요!
src/Navbar/Navbar.js
Outdated
}); | ||
|
||
this.setState = async () => { | ||
const posts = await request(`${documentsUrl}`); |
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 호출 코드는 이름을 지어주고 사용하는것 같아서 이것도 맞춰주면 좋지 않을까요ㅎㅎ
src/Navbar/Navbar.js
Outdated
// 현재 하위 document 가 3개 이상이면 하위 document 값이 삭제가 안되고 있음 | ||
// 하위 document 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.
요거 원인이 뭐라구 생각하시나요~
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.
엇 이거는 지금은 없는 문제인데 기존 코드에 주석을 아직 안지워서 남아있는 것 같습니다.
기존에는 que.shfit를 상단에 배치를하고 while문이 없는 상태여서 que.shift를 했을때 해당 모델을 가져오고 current.documenst.forEach로 돌렸을때 미리 shift한 모델에 한뎁스 documents 에 forEach를 돌리고 있기 때문에 하위 1개만 삭제되는 현상이 발생했습니다.
지금 보니 주석도 잘못표기 되어 있네요.. 3개 이상이면 하위 2개씩이 아니고 하위 1개가 맞습니다.
아래는 기존 코드입니다.
const listDelete = async () => {
const currentDocument = findCurrentId();
const que = [currentDocument];
const queId = [currentDocument.id];
const current = que.shift();
console.log(que);
console.log(current);
current.documents.forEach((el) => {
que.push(el);
queId.push(el.id);
});
console.log(queId);
for (let i of queId) {
console.log(i);
await deleteMethod(i);
}
};
결국에는
1 -> click
2
3
1, 2 만 삭제가 되고 que를 반복하는 루프가 없어서 3을 탐색 못한 것 같습니다.
src/Editor/EditorSubContetn.js
Outdated
// editorDocuments.innerHTML = createSubDocuments(this.state); | ||
// div.appendChild(editorDocuments); |
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/Editor/EditorSubContetn.js
Outdated
@@ -0,0 +1,26 @@ | |||
import { createElement } from '../utils/dom.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.
파일명 오타가 있네요 ㅎㅎ
initialState: initialState, | ||
|
||
onChangeTitle: async ({ id, title }) => { | ||
setTimeout(await putTitleMethod(id, title), 2000); |
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.
확실히 상수로 관리하니깐 코드가 가독성이 좋아지는 것 같습니다 👍 수정하겠습니다 : )
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.
리뷰 완료입니다! 고생하셨어요~
- Editor 컴포넌트를 title , content 2개로 나누었습니다.
좀더 간편하게 ui 을 수정하고 재사용성에 용이하기 위해서 분리했는데 나누지 말고 합치는게 좀더 효율적일까요?
지금 보면 합쳐서 관리하는게 코드도 좀 더 짧아질 것 같은 생각이 들고 불필요한 중복코드들이 안나올듯 싶습니다.
아니면 지금 구조를 유지하고 중복코드를 묶어서 외부로 관리를 해도 괜찮은 방법일까요?
저는 개인적으로 지금 구조도 크게 나쁘지 않은것 같은데요. 꼭 컴포넌트에 위치하지 않아도 되는 코드들은 분리해서 사용하는게 좋아보입니다.
- 첫번째 질문이랑 비슷한 질문이지만 전에 과제를 하다가 의문점이 해결이 안되서 질문남겨봅니다.
투두리스트 과제에서 뷰와 관련 없는 로직을 전부 컴포넌트 외부에 분리하였습니다.
하지만 , 지금 든 생각은 과연 이게 리액트와 맞는 방식일까? 라는 의문점이 생겼습니다.
물론 외부에서 같은 기능을 가진 함수끼리 묶게되면 유지보수에 좋을 것 같은데 이벤트 처리 부분부터
api처리하는 로직까지 전부 묶는게 맞는건지
아니면 유틸 부분만 빼서 지금처럼 외부로 관리하고 나머지는 컴포넌트 내부에서 관리하는게 나을지 의견을 들어보고 싶습니다.
모든 것을 외부로 분리할 필요는 없습니다. 컴포넌트 내부에 있어야 하는 코드들은 같이 두는게 좋아보입니다. 만약 중복 코드가 많다 싶으면 상위 컴포넌트에서 하위 컴포넌트로 해당 함수를 내려주거나 상태를 외부에 두고 해당 상태를 수정할 수 있는 외부 메서드를 사용하는게 좋아보입니다.
유틸로 나눌 수 있는 코드는 분리가 필요하구요(유틸이 뭐인지 기준에 대해서는 고민해보셔야할것 같습니다.)
API 서비스 코드를 호출하거나 상태를 관리하고 이벤트 처리를 하는것은 페이지에 두거나 컴포넌트에 두는 것이 좋고 각각의 컴포넌트의 역할, 재사용할 수 있는데 있어서 적절한 위치를 찾는것이 필요해보입니다.
- 렌더링 될 때마다 화면이 깜빡거리는 증상이 있습니다. 원인을 못찾고 있습니다.
요런것들은 제가 직접 찾아드리면 안될것 같습니다 ㅎㅎ 좀 더 찾아보시고 원인을 공유해주시면 좋을것 같습니다.
렌더링을 하는 부분을 좀 구체적으로 봐야할 것 같습니다.
@off-programmers 오프님 감사합니다. 말씀해주셔서 한방에 고민이 해결되었습니다 : ) |
📌 과제 설명
VaniilaJs Notion Project
👩💻 요구 사항과 구현 내용
글 단위를 Document라고 합니다. Document는 Document 여러개를 포함할 수 있습니다.
컴포넌트 구조
✅ PR 포인트 & 궁금한 점
1
Editor 컴포넌트를 title , content 2개로 나누었습니다.
좀더 간편하게 ui 을 수정하고 재사용성에 용이하기 위해서 분리했는데 나누지 말고 합치는게 좀더 효율적일까요?
지금 보면 합쳐서 관리하는게 코드도 좀 더 짧아질 것 같은 생각이 들고 불필요한 중복코드들이 안나올듯 싶습니다.
아니면 지금 구조를 유지하고 중복코드를 묶어서 외부로 관리를 해도 괜찮은 방법일까요?
2
첫번째 질문이랑 비슷한 질문이지만 전에 과제를 하다가 의문점이 해결이 안되서 질문남겨봅니다.
투두리스트 과제에서 뷰와 관련 없는 로직을 전부 컴포넌트 외부에 분리하였습니다.
하지만 , 지금 든 생각은 과연 이게 리액트와 맞는 방식일까? 라는 의문점이 생겼습니다.
물론 외부에서 같은 기능을 가진 함수끼리 묶게되면 유지보수에 좋을 것 같은데 이벤트 처리 부분부터
api처리하는 로직까지 전부 묶는게 맞는건지
아니면 유틸 부분만 빼서 지금처럼 외부로 관리하고 나머지는 컴포넌트 내부에서 관리하는게 나을지 의견을 들어보고 싶습니다.
3
현재 삭제 기능에서 정상적으로 작동하는데 404 에러가 나오고 있습니다.
url이 잘못된 문제 같은데 원인을 못찾고 있습니다.
제 생각으로는 api 호출 문제이면 삭제 기능이 작동을 하면 안되는걸로 알고 있는데 아니면 리다이렉트에서 발생한 문제일까요..? -- > (해결했습니다)
4
렌더링 될 때마다 화면이 깜빡거리는 증상이 있습니다. 원인을 못찾고 있습니다.