[Mission5/이찬] Project_Notion_VanillaJs 과제 #47
Conversation
1eecan
changed the title
There was a problem hiding this comment.
이찬님! 처음으로 진행하는 개인프로젝트 끝내시느라 고생 많으셨습니다.
새로운 시도를 했었다는 것 자체에 박수를 보내드리고 싶습니다!
제 모자란 경험과 지식으로는 어떤 리뷰를 남길 곳이 없었습니다... (대충 본 거 아님)
그치만 읽으면서 군더더기 없이 잘 싸여있다는 생각이 들었고 실제로 가독성도 좋았습니다!
state가 어떻게 바뀌는지 보기 쉬웠달까요...
아쉬운 점이 있었다면 커밋기록을 보면서 어떻게 컴포넌트를 설계했나 배우고 싶었지만 커밋이 1개였다는 점...
나중에 이찬님 코드 많이 참고하도록 하겠습니다!
수고하셨습니다 😊😊😊
| .map( | ||
| (document) => ` | ||
| <p id="${document.id}" style="text-indent:${ | ||
| document.depth * 10 |
There was a problem hiding this comment.
depth*10은 왜 해주는 건가요? 제가 뭔가 놓친 부분이 있는지.......🐣
There was a problem hiding this comment.
text-indent 속성이 들여쓰기를 해주는 속성입니다! 따라서 depth*10을 해주면 문서목록의 들여쓰기를 해줄 수가 있습니다!
| res = await request("/documents", { | ||
| method: "GET", | ||
| }); | ||
| } |
There was a problem hiding this comment.
res.length===0일 때 새로운 문서가 무조건 생성되는 걸까요?
| import editor from "./component/Editor.js"; | ||
|
|
||
| export default function App({ $target }) { | ||
| this.state = getItem("tempSaveKey") || alert("새로고침을 해주세요"); |
There was a problem hiding this comment.
초기 렌더링을 한 후 storage의 'tempSaveKey' key에 있는 value의 유무를 확인해서 재렌더링을 하면 alert("새로고침을 해주세요")를 하실 필요가 없을 것 같습니다!! 😆
| $documentList.addEventListener("click", (e) => { | ||
| if (e.target.id != "") { | ||
| const { id } = e.target; | ||
| const nextState = onClick(id); | ||
| setItem("tempSaveKey", nextState); | ||
| this.setState(nextState); | ||
| push(`/documents/${id}`); | ||
| } | ||
| }); | ||
|
|
||
| $documentList.addEventListener("click", (e) => { | ||
| const { className } = e.target; | ||
| const { id } = e.target.closest("p"); | ||
|
|
||
| if (className == "postDocument") { | ||
| onPost(id); | ||
| } | ||
| if (className == "deleteDocument") { | ||
| onDelete(id); | ||
| } | ||
| }); |
There was a problem hiding this comment.
$documentList에 대한 click 이벤트 처리를 중복으로 하시는 것 같아 보입니다!
이벤트 위임 처리를 해주면 코드의 중복을 줄일 수 있을 것 같습니다! 😆 🙋🏻♂️
There was a problem hiding this comment.
중첩된 객체를 중접되지 않게 푸는 함수를 생성하셔서 적용시킨 모습이 인상적입니다!! 🤩
다만 데이터가 많아지면 sidebar list를 생성하는데 함수를 한 번 거치는 과정이 필수적이기 때문에 시간적으로 비효율적이라는 생각이 듭니다!!
어떻게 생각하나요? 🤔
| currentDocumentId: | ||
| getItem("tempSaveKey").documentList[ | ||
| getItem("tempSaveKey").documentList.findIndex( | ||
| (document) => document.id == clickedId |
There was a problem hiding this comment.
| (document) => document.id == clickedId | |
| (document) => document.id === Number(clickedId) |
==으로 하면 동일한 값이지만 다른 타입인 값이 같은 값으로 인식이 될 수 있기 때문에 위와 같이 형변환을 해주고 "엄격한" 비교를 해주시면 좋을 것 같습니다. 동치 비교 및 동일성 참고하시면 좋을 것 같아요!! 😀
| const $document = document.createElement("div"); | ||
| $target.appendChild($document); | ||
| this.state = initialState; | ||
| this.setState = async (nextState) => { | ||
| this.state = nextState; | ||
| await editor.setState(this.state); | ||
| await this.render(); | ||
| }; |
There was a problem hiding this comment.
줄바꿈(개행)을 해주시면 가독성 측면에서 좋을 것 같습니다!! 😁
| const documentId = this.state.currentDocumentId; | ||
| const currentDocument = await request(`/documents/${documentId}`, { | ||
| method: "GET", | ||
| }); |
There was a problem hiding this comment.
this.setState에서 documentId에대한 api요청을 request하였는데 state의 변화가 일어나지 않았지만 this.render에서 다시 똑같은 api요청을 request한 것처럼 보여요 👀 따로 빼두시거나 한번만 요청하는 걸로 불필요한 api요청을 줄이는게 나을 듯 합니다! 😄
| this.state = nextState; | ||
| await documentList.setState(this.state); | ||
| await document.setState(this.state); |
There was a problem hiding this comment.
| this.state = nextState; | |
| await documentList.setState(this.state); | |
| await document.setState(this.state); | |
| this.state = nextState; | |
| await documentList.setState(this.state); | |
| await document.setState(this.state); |
p5: 가독성이 좋은 코드를 고민하시는 것 같아서... 저는 이런 경우 개행을 두는 편입니다!! 훨씬 읽기 편한 것 같더라구요 😆(같은 것끼리 묶기!)
There was a problem hiding this comment.
단순 궁금증.
setState가 async 하게 동작하는 이유는 무엇인가요?
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Observer</title> |
There was a problem hiding this comment.
Observer pattern에 대한 고민이 느껴집니다.. 저도 시도해보려다 포기해서 아쉬움이 많습니다.. 😂
| }; | ||
|
|
||
| this.render = async () => { | ||
| if (this.state.documentList.length == 0) return; |
| if (document.documents.length !== 0) { | ||
| nestedObjectArray.push({ | ||
| id: document.id, | ||
| title: document.title, | ||
| depth: depth, | ||
| }); | ||
| recuresiveNestedObjectToArray(document.documents, depth + 1); | ||
| } else { | ||
| nestedObjectArray.push({ | ||
| id: document.id, | ||
| title: document.title, | ||
| depth: depth, | ||
| }); | ||
| } |
There was a problem hiding this comment.
| if (document.documents.length !== 0) { | |
| nestedObjectArray.push({ | |
| id: document.id, | |
| title: document.title, | |
| depth: depth, | |
| }); | |
| recuresiveNestedObjectToArray(document.documents, depth + 1); | |
| } else { | |
| nestedObjectArray.push({ | |
| id: document.id, | |
| title: document.title, | |
| depth: depth, | |
| }); | |
| } | |
| nestedObjectArray.push({ | |
| id: document.id, | |
| title: document.title, | |
| depth: depth, | |
| }); | |
| if (document.documents.length !== 0) { | |
| recuresiveNestedObjectToArray(document.documents, depth + 1); | |
| } |
이렇게 작성해도 괜찮을 것 같아요..! 🤔
| clickedId == "null" | ||
| ? getItem("tempSaveKey").currentDocumentId | ||
| : clickedId, |
There was a problem hiding this comment.
p4 : 값 비교 시 동등 연산자 ( == ) 대신 일치연산자( === ) 을 쓰면 좋을 것 같습니다😊
There was a problem hiding this comment.
ask;
TMI
==와 ===는 똑같이 값을 비교하는 연산자인데 어떤 차이가 있을까요?
- 동작 방식
- 성능
측면에서 어떤 차이가 있을지 쓰윽 살펴보셔도 좋을 것 같아요.
| function recuresiveNestedObjectToArray(nestedObject, depth = 1) { | ||
| nestedObject.forEach((document) => { | ||
| if (document.documents.length !== 0) { | ||
| nestedObjectArray.push({ | ||
| id: document.id, | ||
| title: document.title, | ||
| depth: depth, | ||
| }); |
There was a problem hiding this comment.
단순 궁금한 점입니다! 함수 내에 함수를 선언해서 사용하신 이유가 있나용? 🤔
There was a problem hiding this comment.
안녕하세요 찬님
노션이 정말 어려운 프로젝트라고 생각되는데.. 정말 고생 많으셨어요!
네스티드한 폴더 구조를 정말 멋지게 풀어주셨네요.
이번 코드리뷰는 해답을 드리기 보다는 고민해볼 수 있는 포인트를 남겨 보았어요.
대부분 비슷한 고민을 가지셨을 것 같아 티타임 때 답변을 드릴게요!
구두로 이야기 하는게 훨씬 빠를 것 같기도 하구요!
컴포넌트 사고가 적절히 이루어졌는가?
찬님이 어떤 기준으로 사고를 하셨는지? 잘 전달이 되지 않았던 것 같아요. 혹시 의도를 조금 더 자세히 말씀해주시면 더 원하시는 답변을 드릴 수 있을 것 같아요.
저는 컴포넌트의 역할을 크게 2부분으로 나누어 접근해 보라고 말씀 드리고 싶어요.
- 비즈니스 로직이 존재하는 컴포넌트: API를 fetch 하고 컴포넌트에 연결시키는 역할
- UI로서 동작하는 컴포넌트: 별도의 복잡한 비즈니스 로직이 없고, 단순히 데이터를 받고 보여주거나, UI 동작을 위한 로직이 주가 되는 컴포넌트
를 나눠보는 것도 좋을 것 같아요!
디자인 패턴을 구현하기 위해 생각해 볼만한 점이 있는가?
이번 프로젝트에서 디자인 패턴을 녹여내보고 싶다면
- 지난번에 말씀 드렸던 "옵저버 패턴" (데이터의 변경을 감지하고 효율적으로 전달하기 위한 것)
- 추가로 고민을 해보고 싶다면 "싱글턴 패턴"을 통해 localStorage나 fetch 로직을 개선시켜 봐도 좋을 것 같아요!
가독성이 좋은 코드는 어떻게 짤 수 있을까?
요 부분에 대해 찬님의 기준을 한번 들어보고 싶어요!
어떤 코드가 가독성이 좋다고 생각되나요? 찬님의 코드는 가독성이 좋다고 생각 되시나요? 아쉬운 부분이 있다면 어떤 부분이었는지 쓰윽 짚어보면 좋을 것 같아요.
전반적으로 하나만 딱 짚어서 제안드려본다면. P2 로 적은 App.js에서는 App을 init 하는 과정 코멘트와 같이 코드를 쓰윽 보았을 때 맥락이 맞는 내용만 있는지? 군더더기가 없는지? 쓰윽 본인의 기준을 세워서 나눠보는 연습을 해보시면 좋을 것 같아요!
고생 많으셨습니다~~!
| <div | ||
| id="app" | ||
| style="display: flex; flex-direction: row; justify-content: space-around" | ||
| ></div> |
There was a problem hiding this comment.
ask;
여기에 style을 넣은 이유가 무엇인지 알 수 있을까요??
이 프로젝트에서는 큰 문제가 없긴하나.
SPA를 하다보면 초기 html 세팅 이후 html 파일을 건드리는 경우가 많지 않다보니
여기서 특정 속성을 넣게 되었을 때 원인을 발견하기 쉽지 않은 경우가 꽤 있더라구요.
main.js 부터 보다보니 나는 딱히 뭐 건든게 없는데 왜 레이아웃이 계속 틀어지지..? 같은?
그래서 저는 이런 템플릿 파일에서의 변형은 최소한으로 가져가려 해요.
| import editor from "./component/Editor.js"; | ||
|
|
||
| export default function App({ $target }) { | ||
| this.state = getItem("tempSaveKey") || alert("새로고침을 해주세요"); |
There was a problem hiding this comment.
P5;
소소 리빙 포인트 Nullish coalescing operator (??) 에 대해 알고 계신가요?
||의 경우 0 또는 false, '' 와 같이 boolean으로 형 변환을 한 뒤에 false이면 뒤의 값을 할당하는 구조이다보니 만약 정말 ''이나 0, false가 의도한 값일 경우 의도와 다르게 동작하는 상황이 발생해요!
그에비해 ??의 경우 null이나 undefined와 같이 정말 값이 없을 경우만 핸들링 하죠.
그렇기에 이와 비슷한경우 ??을 추천드려요!
| this.state = nextState; | ||
| await documentList.setState(this.state); | ||
| await document.setState(this.state); |
There was a problem hiding this comment.
단순 궁금증.
setState가 async 하게 동작하는 이유는 무엇인가요?
| const serverDocument = nestedObjectToArray( | ||
| await request("/documents", { | ||
| method: "GET", | ||
| }) | ||
| ); | ||
| setItem("tempSaveKey", { | ||
| documentList: serverDocument, | ||
| currentDocumentId: serverDocument[0].id, | ||
| }); |
There was a problem hiding this comment.
P5;
물론 해당 코드 또한 잘 작성되었지만, request가 하는 일이 워낙 크기 때문에 변수로 한번 짚고 코드를 작성해도 좋았을 것 같아요.
| const serverDocument = nestedObjectToArray( | |
| await request("/documents", { | |
| method: "GET", | |
| }) | |
| ); | |
| setItem("tempSaveKey", { | |
| documentList: serverDocument, | |
| currentDocumentId: serverDocument[0].id, | |
| }); | |
| const serverDocumentList = await request('/document', {...}); | |
| const currentDocumentList = nestedObjectToArray(serverDocumentList); | |
| setItem("tempSaveKey", { | |
| documentList: currentDocumentList, | |
| currentDocumentId: currentDocumentList[0].id, | |
| }); |
| method: "GET", | ||
| }) | ||
| ); | ||
| setItem("tempSaveKey", { |
There was a problem hiding this comment.
P1;
현재 tempSaveKey가 여러 곳에서 쓰이는데 별도의 상수로 관리하면 좋을 것 같아요.
이후에 값이 변경되더라도 최소한의 변경으로 핸들링 할 수 있기 때문이에요.
There was a problem hiding this comment.
P3;
객체지향의 Class의 getter와 setter가 있는 것 처럼 localStorage를 사용해보면 어떨까요?
보다 명시적일 수 도 있을 것 같아서요!
const initStorage = (key: string) => {
return {
get: () => getItem(key);
set: (value) => setItem(key, value);
};
const tempSaveStorage = initStorage('tempSaveKey');
tempSaveStorage.get();
이런 구조는 실제로도 많이 사용되는 패턴이에요.
이를 잘 쓰기 위해서는 클로저 에 대해 공부해보시면 좋아요!
| const splitedPathname = pathname.split("/"); | ||
| const documentId = splitedPathname[2]; |
There was a problem hiding this comment.
ask;
TMI
나중에 path 구조가 /document/:id 에서 /pages/some-list/:id와 같이 바뀌게 될 경우 해당 코드도 맞춰서 고쳐야 할 것 같은데요!
시간이 꽤 지나면 이런 코드가 있었는지? 바로 알아채기 쉽지 않을 것 같아요. 그래서 휴먼에러를 발생할 수 있을 것 같은데요.
이를 예방하거나 변경되었을 때 여기도 같이 챙겨야 함을 어떻게 하면 놓치지 않고 잘 알수 있을까요?
| let timer = null; | ||
|
|
||
| const editor = new Editor({ | ||
| $target: $document, | ||
| initialState: this.state, | ||
| onEditing: async (document) => { | ||
| const documentId = this.state.currentDocumentId; | ||
| if (timer !== null) { | ||
| clearTimeout(timer); | ||
| } | ||
| timer = setTimeout(async () => { | ||
| await request(`/documents/${documentId}`, { | ||
| method: "PUT", | ||
| body: JSON.stringify(document), | ||
| }); | ||
| const nextState = nestedObjectToArray( | ||
| await request(`/documents/${documentId}`, { | ||
| method: "GET", | ||
| }) | ||
| ); | ||
| setItem("tempSaveKey", nextState); | ||
| }, 1000); | ||
| }, |
There was a problem hiding this comment.
P4; ask;
요 timer를 이렇게 써도 되지만 별도의 함수로 뽑아두면 두고두고 쓸 수 있을 것 같아요.
만약 재사용성을 고려한다면 어떻게 만들어 볼 수 있을까요?
| $documentList.addEventListener("click", (e) => { | ||
| if (e.target.id != "") { | ||
| const { id } = e.target; | ||
| const nextState = onClick(id); | ||
| setItem("tempSaveKey", nextState); | ||
| this.setState(nextState); | ||
| push(`/documents/${id}`); | ||
| } | ||
| }); | ||
|
|
||
| $documentList.addEventListener("click", (e) => { | ||
| const { className } = e.target; | ||
| const { id } = e.target.closest("p"); | ||
|
|
||
| if (className == "postDocument") { | ||
| onPost(id); | ||
| } | ||
| if (className == "deleteDocument") { | ||
| onDelete(id); | ||
| } | ||
| }); |
| if (res.ok) { | ||
| return await res.json(); | ||
| } | ||
|
|
There was a problem hiding this comment.
P3;
res.ok가 아닌 경우에는 어떻게 핸들링 하나요?
4xx번대 에러와 5xx번대 에러의 경우 각기 다르게 핸들링 해야할 것 같은데 여기서는 throw new Error("API 처리 중 이상 발생"); 로 퉁쳐지는 것 같아서요!
There was a problem hiding this comment.
ask; 퀴즈
request 함수 관점에서
서버에서 정상적으로 응답이 온 경우!
특히 유저가 실수 해서 난 Error인 4xx번대 에러인 경우 이를 코드상 에러라고 보는 것이 맞을까요?
아니면 정상적으로 동작하는 것으로 보는게 좋을까요?
만약 정상이라고 본다면 어떻게 처리하는 것이 좋을까요?
| }; | ||
|
|
||
| export const push = (nextUrl) => { | ||
| window.dispatchEvent( |
📌 과제 설명
기본적으로 api에서 데이터를 받아와 nestedObjectToArray 함수를 통해 localStorage에 depth를 기록하고
documentList에 가장 최근에 클릭이 된 요소도 localStorage에 저장이 됩니다.
이렇게 저장된 localStorage의 정보를 기반으로 api와 다시 통신을 하여 현재 나타내야할 document들을 렌더링해주는 방식입니다.
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
이번 과제를 하면서 공부하고 싶었던 것은 아래와 같이 크게 세가지가 있었습니다.
이 중에서 디자인패턴을 시도해보겠다고 공부를 하다가 결국 실패하고 부랴부랴 주먹구구식으로 코드를 작성했습니다.
리뷰어님들이 코드를 보시기에 불편하실 것 같아 이점을 미리 사과 드리겠습니다.
제가 이번에 pr을 받고 싶은 부분은 아래의 3가지 요소들에 대한 생각들입니다.
결국에 처음 목표한 3가지를 성공하는 것에 실패했지만, 그래도 완전히 무시하지 않고 제가 할 수 있는 선에서
코드를 작성했습니다. 때문에 리뷰어님들의 작은 조언도 큰 도움이 될 것 같습니다.
감사합니다.