[Mission4/김재현] - Project_Notion_VanillaJS by harryk-im · Pull Request #23 · prgrms-fe-devcourse/FEDC3-4_Project_Notion_VanillaJS

harryk-im · 2022-11-16T15:06:09Z

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

off-programmers · 2022-11-19T08:29:22Z

components/SideBar/SideBarList.js

+        body: JSON.stringify(tempPost),
+      });
+
+      // this.setState()


사용하지 않는 코드들은 제거해주세요 ㅎㅎ

off-programmers · 2022-11-19T08:30:19Z

api/api.js

+
+    throw new Error("API 처리중 뭔가 이상합니다!");
+  } catch (error) {
+    alert(error.message);


alert로 하게 되면 유저들이 이 정보를 보게 될거같은데 이 정보가 유저들이 인지할 필요가 있는 정보일까요?_?

off-programmers · 2022-11-19T08:32:48Z

components/Content/Content.js

+        const isNew = this.state.documentId === "new";
+
+        if (isNew) {
+          const createdPost = await request("/documents", {


아예 코드단에서는 request를 사용해서 url을 넘기는게 아니라 서비스 폴더를 하나 만들거나 해서 거기서 작성하도록 하는것도 좋아보여요.

request(/document); 를 예로 들면 그냥 createPost()같은 메서드를 만들어서 컴포넌트 레벨에서 사용하고 request나 url은 서비스 파일에서 쥐고 관리하는거져. 그럼 훨씬 코드가 직관적으로 보이고 url관리도 좋을것 같습니다.

off-programmers · 2022-11-19T08:33:30Z

components/Content/Content.js

+    $target: $content,
+    initialState: post,
+    onEditing: (post) => {
+      if (timer !== null) {


단지 이 함수 내에서만 사용하는 목적으로 디바운스를 만들었는데 유틸성함수로 뽑아내서 여러군데에서 쉽게 사용할 수 있도록 만들어보세요 ㅎㅎ

p5) 타이머를 null로 사용한건 좋은 거 같아요! 분리 가능하다면 더 좋아질 것 같습니다! 😆

off-programmers · 2022-11-19T08:33:54Z

components/Content/Content.js

+            body: JSON.stringify(post),
+          });
+        }
+      }, 500);


매직넘버는 상수화해서 사용하면 좋을것 같습니다 ㅎㅎ

off-programmers · 2022-11-19T08:34:32Z

pages/App.js

+
+    if (pathname === "/") {
+      await sideBar.setState();
+      await content.setState({ documentId: "new" });


요런 id도 계속 사용하는것 같아서 상수화 하는게 버그 방지에 좋을것 같습니다.

off-programmers · 2022-11-19T08:35:52Z

components/Content/Content.js

+    this.state = nextState;
+    this.render();
+    contentEditor.setState(
+      this.state.document || {


document가 전역으로 있는 키워드이다보니까 다른 이름을 사용하는게 좀 더 좋아보여요!

off-programmers · 2022-11-19T08:36:57Z

components/Content/ContentEditor.js

+    $contentEditor.querySelector("[name=title]").value = this.state.title;
+    $contentEditor.querySelector("[name=content]").value = this.state.content;


돔에 접근하기 위한 목적이라면 별도로 name같은걸로 접근하는거 대신에 id를 사용하는게 좋아보입니다.

off-programmers · 2022-11-19T08:37:17Z

components/SideBar/SideBarList.js

+    const $item = e.target;
+    const { id } = $item.parentElement.dataset;
+
+    if ($item.className === "item-load") {


이런 클래스이름들도 상수화해서 사용해주세요~

off-programmers

리뷰 완료입니다~ 고생하셨어요

yezyvibe

재현님 과제 하시느라 고생 많으셨어요👏👏
이전 투두앱 과제 때보다 바닐라 자바스크립트 실력이 쑥쑥 느신 거 같아요👍👍
요즘 과제가 물밀듯이 나오는데😹 마지막까지 화이팅하세요 :)

yezyvibe · 2022-11-20T05:27:06Z

utils/localStorage.js

+const localStorage = window.localStorage;
+
+export const getItem = (key, defaultValue) => {
+  try {
+    const storedValue = localStorage.getItem(key);
+
+    return storedValue ? JSON.parse(storedValue) : defaultValue;
+  } catch (error) {
+    return defaultValue;
+  }
+};
+
+export const setItem = (key, value) => {
+  localStorage.setItem(key, JSON.stringify(value));
+};
+
+export const removeItem = (key) => {
+  localStorage.removeItem(key);
+};


로컬스토리지 관련 로직은 따로 사용 안 하신 거 같네요!
만약 사용하신다면 setItem, removeItem 함수도 에러 처리를 위해 try, catch문으로 감싸주시는 게 좋을 거 같아요ㅎㅎ

yezyvibe · 2022-11-20T05:33:16Z

pages/App.js

+
+  this.route();
+
+  initRouter(() => this.route());


Suggested change

initRouter(() => this.route());

initRouter(this.route);

라우터 함수만 전달해도 될 거 같습니다!

yezyvibe · 2022-11-20T05:36:05Z

components/Content/ContentEditor.js

+    title: "",
+    content: "",
+  },
+  onEditing,


이벤트 핸들러로 사용하시는 거니까 onEdit이나 onEditContent 처럼 on접두사 뒤에 동사형태가 좋을 거 같습니다ㅎㅎ

yezyvibe · 2022-11-20T05:38:07Z

components/SideBar/SideBarItem.js

+export default function SideBarItem(item) {
+  return `
+    <li data-id="${item.id}">
+      <span class="item-load">
+        ${item.title}
+      </span>
+      <button class="item-remove">-</button>
+      <button class="item-add">+</button>
+      <ul class="sidebar-list-ul">
+        ${item.documents.length > 0 ? item.documents.map((item) => SideBarItem(item)).join("") : ""}
+      </ul>
+    </li>
+  `;
+}


비구조할당으로 item에서 id, title, documents를 빼서 바로 사용해도 좋을듯 싶습니다ㅎㅎ

yezyvibe · 2022-11-20T05:43:27Z

components/SideBar/SideBarList.js

+  $sideBarList.addEventListener("click", async (e) => {
+    const $item = e.target;
+    const { id } = $item.parentElement.dataset;
+
+    if ($item.className === "item-load") {
+      push(`/documents/${id}`);
+
+      return;
+    }
+
+    if ($item.className === "item-remove") {
+      await request(`/documents/${id}`, {
+        method: "delete",
+      });
+
+      push(`/`);
+
+      return;
+    }
+
+    if ($item.className === "item-add") {
+      const tempPost = {
+        title: "",
+        parent: id,
+      };
+      const createdPost = await request("/documents", {
+        method: "post",
+        body: JSON.stringify(tempPost),
+      });
+
+      // this.setState()
+      push(`/documents/${createdPost.id}`);
+    }
+  });
+}


이벤트 핸들러 같은 경우 모든 케이스를 if문으로 분기하시는 방법은 좋지만, 로직이 조금 길어질 때는 if 안의 로직을 따로 함수로 분리하셔도 좋을 거 같아요😊😊

yezyvibe · 2022-11-20T05:59:03Z

components/Content/Content.js

+  const post = {
+    title: "",
+    content: "",
+  };
+
+  let timer = null;
+
+  const contentEditor = new ContentEditor({
+    $target: $content,
+    initialState: post,


contentEditor의 초기값으로 post 변수를 만들어 넣어주셨는데, 이 post는 변수는 1회성으로 사용하시는 거 같아서 생성자 함수의 인자 initialState에 초기값을 직접 넣어주셔도 될 거 같고, 또 ContentEditor에도 이미 디폴트 파라미터를 정의하셔서 생략도 가능할 거 같습니다.
그리고 만약 초기값을 변수로 만들어서 사용하신다면 아래 60번째 줄, contentEditor.setState에서는 'document'로 변수를 전달하셨기 때문에 변수명이 서로 상이해서 통일하시는 게 좋을 거 같습니다! :))

zerosial · 2022-11-20T15:46:38Z

components/Content/Content.js

+        await fetchPost();
+      }
+
+      return;


p5) 마지막 리턴은 없어도 작동할 거 같아요!

zerosial · 2022-11-20T15:50:10Z

utils/localStorage.js

@@ -0,0 +1,19 @@
+const localStorage = window.localStorage;


p5) 아마 기능 추가를 하려고 만들다가 시간이 부족해서 못하신 것 같내요 😂

zerosial

안녕하세요 재현님! 코드리뷰하러 왔습니다 😉

코드가 예전보다 전반적으로 엄청 깔끔해 지신 것 같아요. 👍👍

좀 더 다양한 기능과 코드를 실험해보고 코드리뷰를 받아보는 것도 좋을 것 같습니다!

저도 이번에 작성하면서 엄청 여러지식도 늘고 되게 재밌었거든요 일단 만들어보니.. ㅋ.ㅋ

다른 팀에서도 파이팅 하세요 !! 😆

ghost

안녕하세요 재현님, 코드리뷰를 처음이자 마지막으로 해드리네요 ㅎ.ㅎ
다른 분들이 다 말씀해주셔서 딱히 드릴 말씀이 없지만 마지막 리뷰 해드릴게요 : )
1달동안 수고하셨습니다~

ghost · 2022-11-20T16:03:51Z

components/Content/Content.js

+    if (this.state.documentId !== nextState.documentId) {
+      this.state = nextState;
+
+      if (this.state.documentId === "new") {


this.stat.documentId 중첩해서 나오는데
상수화 시켜서 사용하면 좀더 직관적으로 보일 듯 하네요 ㅎㅎ

ghost · 2022-11-20T16:04:01Z

components/SideBar/SideBarList.js

+  };
+
+  this.render = () => {
+    if (!this.state) return;


이부분은 굳이 없어도 될 것 같습니다 ㅎㅎ 아니면 따로 함수로 만들어서 관리하는 것도 좋겠네요 : )

ghost · 2022-11-20T16:04:07Z

components/SideBar/SideBarList.js

+
+      push(`/`);
+
+      return;


return 없어도 될 것 같아요 ㅎㅎ

ghost · 2022-11-20T16:04:09Z

components/SideBar/SideBarList.js

+        title: "",
+        parent: id,
+      };
+      const createdPost = await request("/documents", {


"/documents" 상수화 하시면 좋을 것 같습니다 ㅎㅎ

ghost · 2022-11-20T16:04:47Z

style/style.css

@@ -0,0 +1,41 @@
+.layout-main {


css 모듈화 해보시는거 어떨까요? 폴더구조가 좀 더 직관적으로 보일 듯 합니다.ㅎㅎ

ghost · 2022-11-20T16:17:40Z

components/Content/ContentEditor.js

+
+  this.render();
+
+  $contentEditor.addEventListener("keyup", (e) => {


이벤트 리스너 함수로 묶어서 호출하는 방식으로
확장성에 대비해서 재사용하기 쉽게 관리하는 것도 괜찮아 보입니다. 그리고 함수로 묶으면 네이밍만 보고서도
코드를 읽는 입장에서 바로 알 수 있습니다. ㅎㅎ

ghost · 2022-11-20T16:34:40Z

pages/App.js

@@ -0,0 +1,40 @@
+import SideBar from "../components/SideBar/SideBar.js";


App이 page 폴더에 있는건 살짝 투머치인것 같아요
디렉터리 구조 관련 링크 넣어둘게요~
아주 좋은 디렉터리 구조 관련 블로그글 여기 있네요!

리액트 디렉터리 구조

이거는 tmi 유투브 링크입니다./
https://www.youtube.com/watch?v=UUga4-z7b6s

ghost · 2022-11-20T16:37:52Z

components/SideBar/SideBar.js

+
+export default function SideBar({ $target }) {
+  const $sideBar = document.createElement("div");
+  $sideBar.classList.add("layout-sidebar");


classList.add 보다 className이 좀더 괜찮아 보이네요 ㅎㅎ

ghost · 2022-11-20T16:40:07Z

components/Content/ContentEditor.js

+      class="editor-content"
+      name="content"
+    >
+      ${this.state.content}


this.state.content 말고 다른 대안으로

const {content} = this.state

이것도 괜찮아 보이네요 ㅎㅎ

ghost · 2022-11-20T16:51:26Z

components/Content/ContentEditor.js

+    this.setState(nextState);
+    onEditing(this.state);
+  });


onEditing에서 nextState 바로 넘겨줘도 괜찮아 보이네요 ㅎㅎ

Feat: 필수 요구 사항 일부 구현 ( child node 표시기능누락. )

adf99cf

harryk-im added 과제 리뷰 요청 피드백 요청 🔥 매운맛 오프팀 labels Nov 16, 2022

harryk-im requested a review from off-programmers November 16, 2022 15:06

harryk-im self-assigned this Nov 16, 2022

Feat: 필수 요구 사항 구현

02c60ec

off-programmers reviewed Nov 19, 2022

View reviewed changes

off-programmers approved these changes Nov 19, 2022

View reviewed changes

yezyvibe reviewed Nov 20, 2022

View reviewed changes

zerosial reviewed Nov 20, 2022

View reviewed changes

components/Content/Content.js

await fetchPost();

}

return;

Copy link

zerosial Nov 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p5) 마지막 리턴은 없어도 작동할 거 같아요!

zerosial reviewed Nov 20, 2022

View reviewed changes

ghost reviewed Nov 20, 2022

View reviewed changes

		$contentEditor.querySelector("[name=title]").value = this.state.title;
		$contentEditor.querySelector("[name=content]").value = this.state.content;


		this.render();

		$contentEditor.addEventListener("keyup", (e) => {

		@@ -0,0 +1,40 @@
		import SideBar from "../components/SideBar/SideBar.js";

Conversation

harryk-im commented Nov 16, 2022

📌 과제 설명

👩‍💻 요구 사항과 구현 내용

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

off-programmers left a comment

Choose a reason for hiding this comment

Uh oh!

yezyvibe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zerosial left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Nov 20, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Nov 20, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

ghost Nov 20, 2022 •

edited by ghost

Loading

ghost Nov 20, 2022 •

edited by ghost

Loading