Skip to content
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

[Mission5/김정호-p] Project_Notion_VanillaJs 과제 #49

Open
wants to merge 18 commits into
base: 4/#5_JungHoKim-p
Choose a base branch
from

Conversation

perfumeplaylist
Copy link

@perfumeplaylist perfumeplaylist commented Jul 6, 2023

📌 과제 설명!

제목 없는 다이어그램

  • 이번 과제는 낙관적 업데이트를 최대한 신경쓰면서 노션클로닝을 구현해보았습니다.
  • Button,Header 컴포넌트는 최대한 활용할 수 있게 구현해보았습니다.
  • 데이터 구조 때문에 재귀함수를 이용하였습니다.

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

기본 요구사항

  • 화면 좌측에 Root Documents를 불러오는 API를 통해 루트 Documents를 렌더링합니다.
  • Root Document를 클릭하면 오른쪽 편집기 영역에 해당 Document의 Content를 렌더링합니다.
  • 해당 Root Document에 하위 Document가 있는 경우, 해당 Document 아래에 트리 형태로 렌더링 합니다.
  • Document Tree에서 각 Document 우측에는 + 버튼이 있습니다. 해당 버튼을 클릭하면, 클릭한 Document의 하위 Document로 새 Document를 생성하고 편집화면으로 넘깁니다.
  • 편집기에는 기본적으로 저장 버튼이 없습니다. Document Save API를 이용해 지속적으로 서버에 저장되도록 합니다.
    • History API를 이용해 SPA 형태로 만듭니다.
    • 루트 URL 접속 시엔 별다른 편집기 선택이 안 된 상태입니다.
    • /documents/{documentId} 로 접속시, 해당 Document 의 content를 불러와 편집기에 로딩합니다.

보너스 요구사항

  • 편집기 최하단에는 현재 편집 중인 Document의 하위 Document 링크를 렌더링하도록 추가합니다.
  • 현재 가리키는 페이지를 삭제시 루트로 이동하는 기능 추가하였습니다.

아쉬운 점

  • 전역으로 상태를 관리하고 낙관적 업데이트를 위해서 2번씩 setState를 하다보니깐 리렌더링하게 되어서 아쉬웠습니다.
  • 상태관리와 로직에 대해서 고민하다 보니깐 스타일을 미완성이 되어서 아쉬웠습니다.

✅ PR 포인트 & 궁금한 점

1.낙관적 업데이트를 위해서 전역 상태로 관리 했는데 이러면 App에 종속적인 건가요??
2.상태를 낙관적으로 업데이트 하기 위해서 재귀함수로 구현 했는데 영향이 없는지 궁금합니다!
3.코드의 일관성이 잘 처리되었는지 궁금합니다.
4.전역으로 상태를 관리하고 낙관적 업데이트를 위해서 2번씩 setState를 하다보니깐 리렌더링을 하는데 이걸 해결 하는 방법이 있는지 궁금합니다!
5.좀 더 나은 상태관리를 어떻게 하면 할 수 있는지 궁금합니다!

@perfumeplaylist perfumeplaylist requested a review from pocojang July 6, 2023 15:00
@perfumeplaylist perfumeplaylist changed the title 4/#5 jung ho kim p working [Mission5/김정호] Project_Notion_VanillaJs 과제 Jul 6, 2023
@perfumeplaylist perfumeplaylist changed the title [Mission5/김정호] Project_Notion_VanillaJs 과제 [Mission5/김정호-p] Project_Notion_VanillaJs 과제 Jul 6, 2023
@perfumeplaylist perfumeplaylist self-assigned this Jul 7, 2023
@suyeon1218
Copy link

정호님 마지막까지 구현 열심히 하시는 모습이 인상 깊었습니다! 그에 반해 저는 안되는 건 그냥 넘어가버리고 했어서... 이번에 정호님 보며 많이 배웠습니다! 이번 과제도 수고 많으셨어요!

src/App.js Outdated
Comment on lines 12 to 14
import EditPage from "./components/page/EditPage.js";
export default function App({
$target,

Choose a reason for hiding this comment

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

import 밑 부분 함 줄 띄워주시는 거 깜빡하신 것 같아요~!

Copy link
Author

Choose a reason for hiding this comment

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

헐 이런 사소한 부분 잡아주시는거 너무 좋습니다ㅠㅠㅠ감사합니다 👍

src/main.js Outdated

new App({
$target: $app,
initalState: { selectedId: null, posts: [], post: {} },

Choose a reason for hiding this comment

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

posts 와 post 가 걸핏하면 헷갈리기 쉬운 변수명이라고 생각합니다! post 가 특정 포스트의 정보를 받아온다면 selectedPost 나 currentPost 같은 명시적인 변수명을 사용해보시는 게 어떨까요!

Copy link
Author

Choose a reason for hiding this comment

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

완전 좋은것 같습니다!!!!!!고민 했었는데 이제 확실하게 고쳐야겠네요!

src/App.js Outdated
Comment on lines 13 to 15
export default function App({
$target,
initalState = {

Choose a reason for hiding this comment

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

main.js 에서 App 컴포넌트를 호출할 때 이미 initialState 의 값을 결정해서 전달하는데, 여기서 initialState 를 한 번더 default 값으로 설정해주신 이유가 있으실까요!

src/App.js Outdated
post: {},
},
}) {
if (!new.target)

Choose a reason for hiding this comment

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

new 키워드를 쓰지 않고 호출한 상태라서 예외처리를 해주신 거라면 중괄호 {}... 를 사용해주시는 게 코드블록의 시작과 끝을 알 수 있어서 좋을 것 같습니다!

Choose a reason for hiding this comment

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

저도 이건 중요하다고 느껴지네요..ㅠㅠ

Copy link
Author

Choose a reason for hiding this comment

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

오 감사합니다ㅠㅠ신경 못쓰고 지나갈뻔 했는데 잡아주셔서 감사합니다! 💯

src/App.js Outdated
this.setState = (nextState) => {
this.state = { ...this.state, ...nextState };
postPage.setState({
selectedId: this.state.selectedId || null,

Choose a reason for hiding this comment

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

OR 연산자 사용하신 거 멋지십니다! 배워가요~!

Choose a reason for hiding this comment

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

?? (nullish) 는 어때요?

Copy link
Author

Choose a reason for hiding this comment

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

오... 감사합니다! null 병합 연산자에 대해서 자세히 알아보겠습니당! js 신기한 연산자가 너무 많네요 :)

});
};

const onAdd = async (id) => {

Choose a reason for hiding this comment

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

onAdd, onDelete 와 같이 각 함수가 하는 일에 따라 이름을 지어 분류해주셔서 읽기 편합니다! 👍

Copy link
Author

Choose a reason for hiding this comment

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

다 수연님 덕분입니다... 🥇

$header.textContent = "Untitle";
return;
}
$header.textContent = `${this.state.title}`;

Choose a reason for hiding this comment

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

header 에선 state 가 자주 변동될 일이 없을 것 같은데 그 이 경우 Header 내에서 직접 title 을 지정해주는 것도 나쁘지 않을 것 같습니다!

$header.textContent = '정호의 Notion';

같은 식으로요!

this.render();
};

const createDocument = (

Choose a reason for hiding this comment

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

제가 보기엔 createDocument 가 함수의 역할도 하지만 컴포넌트 생성의 역할이 더 커보여서 1. 컴포넌트 형식으로 이름을 지어주거나 2. 따로 컴포넌트를 만들어 빼주는 거나 3. render 안에 모두 작성하는 것도 좋을 것 같습니다!

};

const createDocument = (
{ id, title, documents },

Choose a reason for hiding this comment

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

documents 의 경우 해당 프로퍼티가 해당 document 의 Child 를 가리키는지 명확하지 않아 보여요! 변수를 만들어서 child 임을 확실히 알 수 있게 해준다면 좋을 듯 합니다!

const {id, title} = document;
const childrenDoc = document.documents;

if (childrenDoc.length) {
    // arr.push() ...
}

Copy link
Author

Choose a reason for hiding this comment

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

이런게 진짜 기본인데 😢 😢 잘 집어주셔서 감사합니다 :):)


const createDocument = (
{ id, title, documents },
arr,

Choose a reason for hiding this comment

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

arr 가 여러 html 태그들을 담는 변수라면, 좀 더 명확한 변수명이 좋을 거 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

넵!!! 바로 수정했습니다! 변수명 신경...메모 ⭐

Comment on lines 83 to 84
if (!$li) return;
const {

Choose a reason for hiding this comment

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

이 경우 다음과 같이 쓸수 있습니다!

const { documentId } = $li.dataset;

Copy link
Author

Choose a reason for hiding this comment

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

오 저런 식으로 작성하는게 확실히 눈에 더 들어오네요!! 완전 센스가 장난 아니십니다!!🥇


const onItemHover = (target) => {
const $li = target.closest("li");
if ($li) $li.classList.toggle("block");

Choose a reason for hiding this comment

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

우왕 classList 에 toggle 이라는 메서드가 있군요?! 처음 알아갑니다!!

ToggleItem(event.target, id);
} else if (className === "delete") {
onDelete(id);
} else if (className === "item" || className === "item block") {

Choose a reason for hiding this comment

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

정호님...! 도큐먼트 상세 정보를 보는 이벤트가 제대로 동작하지 않는 것 같아요...!!

Copy link
Author

Choose a reason for hiding this comment

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

바로 고쳤습니다!!제가 너무 className 허술하게 관리해서 그런것 같네요ㅠㅠㅠ 😭

@@ -0,0 +1,72 @@
import { push } from "../../util/router.js";

Choose a reason for hiding this comment

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

메소드는 메소드끼리, 컴포넌트는 컴포넌트끼리 묶어서 import 하는 게 클린 코드 작성에 도움이 됩니당!

import { push } from "../../util/router.js";
import { request } from "../../util/api.js";
import { UNTITLE } from "../../constant.js";
import { isSuitableId } from "../../util/prevent.js";
import ButtonContainer from "../EditComponents/ButtonContainer.js";
import Editor from "../EditComponents/Editor.js";
import Header from "../Header.js";

Copy link
Author

Choose a reason for hiding this comment

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

오 진짜 꿀이 가득담긴 팁 감사합니다!!!!다음부터는 무조건 저렇게 작성하겠습니다!!! 👍

}
};

const loadPost = async () => {

Choose a reason for hiding this comment

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

loadPost 라는 작명 좋은 것 같아요...! 추상화를 열심히 하셨군요 정호님 👍

Copy link

@backward99 backward99 left a comment

Choose a reason for hiding this comment

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

state를 어떻게 관리해야 하는지 감이 안잡혀서 추가 삭제 하는 부분은 낙관적 업데이트를 못했는데 반성해야겠습니다.. 정호님 코드를 보면서 저도 함수를 작게 분리하면서 알맞은 이름을 붙이는 연습도 해야겠다 생각이 들었습니다. 노션 클로닝 과제 하느라 수고하셨습니다!

Comment on lines +88 to +91
const visitedDocumentsId = getItem(VISITED_LOCAL_KEY, []).filter(
(visited) => visited !== id
);
setItem(VISITED_LOCAL_KEY, visitedDocumentsId);

Choose a reason for hiding this comment

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

해당 부분은 fetchDelete에도 있는데 두 번 사용하신 이유가 있는지 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

App 상태를 업데이트 하고 삭제된걸 바로 보여지기 위해서 사용했습니다!(낙관적 업데이트 의도한거라고 생각하고 시도했습니다!) :)

}
const [, , postId] = pathname.split("/");
const visitedDocumentsId = getItem(VISITED_LOCAL_KEY, []);
setItem(VISITED_LOCAL_KEY, [...visitedDocumentsId, parseInt(postId)]);

Choose a reason for hiding this comment

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

span 부분을 클릭하면 하위 목록까지 한번에 보이게 설정하신 건가요?

Copy link
Author

Choose a reason for hiding this comment

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

넵 저런식으로 처리 했는데 아직 고쳐야할게 많아보입니다ㅠㅠㅠ 과제 기한이 끝나더라도 무조건 해결하고 알려드리겠습니다! 👍

</li>
`);

if (documents.length) {

Choose a reason for hiding this comment

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

여기에서는 isLength를 사용하지 않은 이유가 있나요!

Copy link
Author

Choose a reason for hiding this comment

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

오 저 부분은 제가 깜빡하고 넘어갔네요ㅠㅠㅠ바로 수정하겠습니다! 찾아주셔서 감사합니다!! :)

Copy link

@eugene028 eugene028 left a comment

Choose a reason for hiding this comment

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

정호님! 이번 과제를 하시면서 여러번 코드를 갈아엎었다고 말씀하시면서 고민도 많이 하시고, 힘들어 하셨던 모습을 봤어서 그런지 결국 거의 모든 기능을 성공적으로 구현해낸 코드를 보면서 정호님 짱이다!!! 너무 잘 하셨는데?!?! 하면서 감탄하면서 봤던 것 같아요.ㅎㅎ
고민 많이 하시고 만드신 것 같아서 무슨 의미인지 코드를 하나하나 읽어보면서 저도 공부할 수 있는 리소스를 많이 얻어갈 수 있었던 것 같습니다.
이번 프로젝트 정말 쉽지 않았는데 이렇게 포기하지 않고 결과물을 만들어 내신 것 자체가 너무 대단한 것 같아요. 앞으로 어떤 힘든 일들이 있어도 그때 이렇게 극복했었지! 라는 자신감을 가지고 앞으로 마주하게 될 과제와(?) 고민들도 지금처럼 잘 헤쳐나가실 것 같아서 앞으로가 더욱 기대되는 것 같아요.
파이팅입니다 !!!

Choose a reason for hiding this comment

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

reset css까지 꼼꼼히 챙겨주셨군요!! 짱입니다 👍

Copy link
Author

Choose a reason for hiding this comment

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

유진님에 비하면 아주 초라합니당....><

setItem(VISITED_LOCAL_KEY, [...visitedDocumentsId, id]);
const tempData = [...this.state.posts];
addDocuments(tempData, id, newData);
this.setState({ selectedId: "new", posts: tempData });

Choose a reason for hiding this comment

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

아직 Id를 발급받지 않았을 때에는 new 라는 아이디를 이용하여 컨트롤 한 뒤에, 이후 API 호출을 통하여 문서를 생성하면 해당 Id로 변경을 해 주는 것이 인상깊습니다!

await fetchDelete(`/documents/${id}`, { method: "DELETE" }, id, filterPost);
};

const fetchDelete = async (url, options, id, filterPost) => {

Choose a reason for hiding this comment

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

fetchDelete , fetchUpdate, fetchAdd와 같은 함수는 App.js 파일에 모든것을 정의하기보다는 api 모듈로 관리하는 방식으로 생각하여 해당 파일과 분리하면 좋을 것 같습니다 :) 참고 링크 남깁니다

https://velog.io/@hsk10271/TIL-33
https://ghost4551.tistory.com/163

Copy link
Author

Choose a reason for hiding this comment

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

오 진짜 감사합니다ㅠㅠㅠ 이거는 진짜 귀하네요 👍 👍 잘 읽어보고 무조건 적용시켜보겠습니다!

onDelete,
});

const $page = document.createElement("section");

Choose a reason for hiding this comment

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

section 태그로 생성하신 이유가 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

저 페이지는 본문과 연관된 한 챕터인 단위 같아서 section태그로 작성해보았습니다!혹시 잘못된 점이 있으면 알려주십쇼!!
이 영상보고 작성했습니다!
https://www.youtube.com/watch?v=T7h8O7dpJIg&t=407s&ab_channel=%EB%93%9C%EB%A6%BC%EC%BD%94%EB%94%A9

src/App.js Outdated
@@ -0,0 +1,162 @@
import PostPage from "./components/page/PostPage.js";

Choose a reason for hiding this comment

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

보통 기능이나 블록 단위를 개발하는 컴포넌트 파일을 components폴더에 정리하고, 순수히 페이지를 나타내는 컴포넌트들은 pages 폴더에 정리하는 걸로 알고 있어요!
그래서 폴더 구조가 components/pages/,..로 가기 보다는 그 두개의 폴더를 분리하면 더욱 좋을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

오...확실히 개발을 많이하신 바이브가 여기까지 느껴집니다 💯 참고해서 수정하겠습니다!

visitedDocumentsId.indexOf(id) > -1 ? "block" : "none"
}" class="itemList">`
);
for (const document of documents)

Choose a reason for hiding this comment

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

for문보다는 forEach문을 사용해보는 것이 어떨까요?! for문이 속도 면에서는 빠를 수 있어도, 가독성 면에서 forEach가 Javascript를 사용하는 입장에서 더욱 가독성이 좋을 것 같습니다!

const id = parseInt(documentId);
if (className === "add") {
onAdd(id);
} else if (className === "toggle " || className === "toggle active") {

Choose a reason for hiding this comment

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

이 경우에는 classNametoggle만이 존재하여도 else if문을 통과할 수 있게끔 만들어 주어도 될 것 같네요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

바로 수정하겠습니다!!!

this.state = initalState;

this.setState = async (nextState) => {
if (this.state.selectedId === nextState.selectedId && nextState.post) {

Choose a reason for hiding this comment

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

nextState.post 의 내용도 selectedId와 더불어 체크되어야 하는 이유가 궁금합니다!

onDelete,
});

new Button({

Choose a reason for hiding this comment

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

Button을 독립적인 컴포넌트로 분리해서 재사용할 수 있게 만드셨군요 !! 👍

return;
}

const $fakeDocument = document.createDocumentFragment();

Choose a reason for hiding this comment

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

처음 보는 메서드라서 저도 공부 해봐야겠군요 😮 가상의 노드를 만들어서 임시방편으로 타겟에 넣어두는 방식의 설계 배워갑니다 정호님

Choose a reason for hiding this comment

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

굿 좋은 활용 방법이에요.
하지만 fake 라는 네이밍은 애매한 것 같네용

Copy link

@SongInjae SongInjae left a comment

Choose a reason for hiding this comment

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

  • 작성 중일 때, 다른 항목들로 넘어가는 기능이 구현되면 더 좋을 것 같습니다:)
  • 나중에 css 부분을 수정하실 때 '+'와 'x'가 텍스트의 길이에 맞춰 이동하는 것보다는 한 곳에 고정되어 있는 것이 UI에 좋을 것 같습니다
  • 폴더구조를 보면 component에 모든 것이 다 들어있는 구조인 것 같습니다! 나중에 프로젝트가 조금 더 커진다면 폴더구조가 component를 중심으로 많이 깊어질 것 같아서 폴더구조에 조금 변화가 필요할 것 같습니다!

@@ -0,0 +1,43 @@
html, body, div, span, applet, object, iframe,

Choose a reason for hiding this comment

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

이렇게 모든 것을 추가해줘도 되지만 * {} 안에 넣어준다면 더 깔끔하게 추가하실 수 있습니다!

flex-direction: row;
}

button{

Choose a reason for hiding this comment

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

현재 사이드바에 li에 mouseover를 했을 때 li의 크기보다 버튼의 크기가 큽니다! 버튼의 크기와 높이를 지정해주거나 다른 방법이 필요해보입니다!

Copy link

@pocojang pocojang left a comment

Choose a reason for hiding this comment

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

안녕하세요
정호님~~~

다이어그램까지 포함해서 PR정리한게 너무 좋구만요

피드백

  • 폴더 구조는 나름 잘 구성하신 것 같습니다. 👍
  • src/App.js 에 불필요한 코드가 너무너무 많아요ㅠㅠ분리하는게 좋습니다.

함수로 생성자 함수를 구현한 이유가 무엇일까요?

조금 아쉽네요
과거의 JS는 Instance 생성을 돕는 Class가 존재하지 않아서 이런 생성자 함수를 사용했었습니다.
여기서 몇가지 중요 포인트를 뽑아본다면요.
이 두가지인데 꼭 알고 넘어가셨으면 좋겠습니다.

이미 대체할 수 있는 Class가 등장했는데 굳이 생성자함수를 사용해야했을까?
이렇게 될 경우 메서드 메모리가 어떻게 될까?
이외에 중요한 포인트는 코멘트 리뷰 남겨놨습니다.

Q&A

Q1.낙관적 업데이트를 위해서 전역 상태로 관리 했는데 이러면 App에 종속적인 건가요??

음.. 낙관적 업데이트와 전역 상태 관리는 어떤 관계가 있을까요? 잘 모르겠어요
App과도 관계가 없는 것 같은데요?
낙관적 업더이트의 맥락을 이해하고 하신 질문인지 잘 모르겠습니다!

Q2.상태를 낙관적으로 업데이트 하기 위해서 재귀함수로 구현 했는데 영향이 없는지 궁금합니다!

재귀는 좋은 프론트엔드 입장으로 위험하지만 자주 사용되는 패턴이기도해요.
에러처리만 더욱 견고하게 하시면 좋겠습니다.

Q3.코드의 일관성이 잘 처리되었는지 궁금합니다.

폴더 구조까지는 괜찮았습니다!
다만 상태를 관리하는 방법에 대한 일관성은 애매한 것 같네요

Q4.전역으로 상태를 관리하고 낙관적 업데이트를 위해서 2번씩 setState를 하다보니깐 리렌더링을 하는데 이걸 해결 하는 방법이 있는지 궁금합니다!

순수 JS이기때문에 이런 대비 코드를 위한 설계를 하지 않는 이상 손수 구현해야겠죠?
현재는 어차피 수동 리렌더 구조라.. 애매한 질문이라 생각이 듭니다.

Q5.좀 더 나은 상태관리를 어떻게 하면 할 수 있는지 궁금합니다!

일관성을 잘지키시고요.
불필요한 상태를 애초에 만들지 않는 것을 추천합니다!


과제 고생하셨습니다!!

Choose a reason for hiding this comment

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

reset.css 까지 굳이 필요할까 싶긴하네요 ㅎㅎ

@@ -0,0 +1,14 @@
<!DOCTYPE html>
<html lang="ko">

Choose a reason for hiding this comment

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

🇰🇷

<main id="app"></main>
<script src="/src/main.js" type="module"></script>
</body>
</html>

Choose a reason for hiding this comment

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

EOL 반복.. 😢

src/App.js Outdated
post: {},
},
}) {
if (!new.target)

Choose a reason for hiding this comment

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

저도 이건 중요하다고 느껴지네요..ㅠㅠ

src/App.js Outdated
this.setState = (nextState) => {
this.state = { ...this.state, ...nextState };
postPage.setState({
selectedId: this.state.selectedId || null,

Choose a reason for hiding this comment

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

?? (nullish) 는 어때요?

Comment on lines +4 to +5
if (!title || !title.length) return false;
return true;

Choose a reason for hiding this comment

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

Suggested change
if (!title || !title.length) return false;
return true;
return Boolean(title?.length);

export const getItem = (key, defaultValue) => {
try {
const resultItem = JSON.parse(storaged.getItem(key));
return resultItem ? resultItem : defaultValue;

Choose a reason for hiding this comment

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

JSON.parse 하기 전에 검사하는게 좋지 않을까요?

const resultItem = JSON.parse(storaged.getItem(key));
return resultItem ? resultItem : defaultValue;
} catch (e) {
alert(e.message);

Choose a reason for hiding this comment

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

유저가 알 필요는 없을 것 같기도 하네요?

};

export const filterDocument = (posts, { title, id }) => {
for (let post of posts) {

Choose a reason for hiding this comment

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

Suggested change
for (let post of posts) {
for (�const post of posts) {

src/constant.js Outdated

Choose a reason for hiding this comment

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

불필요한 주석은 제거합시다~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants