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/김유진] Project_Notion_VanillaJs 과제 #44

Open
wants to merge 16 commits into
base: 4/#5_eugene028
Choose a base branch
from

Conversation

eugene028
Copy link

@eugene028 eugene028 commented Jul 6, 2023

📌 과제 설명

바닐라 JS를 이용하여 Notion을 클론하였습니다.

  • 글 생성하기 기능 (루트문서, 하위문서 생성)
  • 글 삭제하기
  • 제목 및 글 내용 작성 시 자동완성 기능 구현
  • 하위 문서에 대한 링크 랜더링

🔗 배포링크

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

노션 클론 페이지 구조

image
최대한 부모 컴포넌트에 의존하기보다는 각각의 컴포넌트트와 페이지가 독립적으로 API를 호출하고 state를 관리할 수 있도록 하였습니다.
자신의 역할에 따른 상태관리만을 할 수 있도록 설계해 보고자 노력했습니다.

명세사항

  • 글 단위를 Document라고 합니다. Document는 Document 여러개를 포함할 수 있습니다.

  • 화면 좌측에 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를 불러와 편집기에 로딩합니다.

보너스 요구사항 📚

  • 기본적으로 편집기는 textarea 기반으로 단순한 텍스트 편집기로 시작하되, 여력이 되면 div와 contentEditable을 조합해서 좀 더 Rich한 에디터를 만들어봅니다.
  • 편집기 최하단에는 현재 편집 중인 Document의 하위 Document 링크를 렌더링하도록 추가합니다.
  • 편집기 내에서 다른 Document name을 적은 경우, 자동으로 해당 Document의 편집 페이지로 이동하는 링크를 거는 기능을 추가합니다.

✅ 피드백 반영사항

  • 코드의 일관성을 위해 중괄호, validation 을 유지할 수 있는 코드를 추가적으로 작성했습니다.
  • isOpenStatedepth가 의도대로 작동할 수 있도록 해당 state를 유지하는 로직을 수정했습니다.
  • javascript의 메서드 사용 중 미흡한 지점이 있어 개선하였습니다.
  • DOM을 직접 조작하는 것을 최소화하기 위한 작업을 진행 중에 있습니다.

✅ PR 포인트 & 궁금한 점

  • Rich한 에디터를 생성하려고 하였으나 (노션처럼 ##, #를 입력하면 자동으로 마크다운 형식으로 변환) 해당 기능을 구현하는 데 아직 공부가 미진한 듯 하여 완성하지 못했습니다. ㅠㅠ velog가 어떻게 만들어졌는지 래퍼런스를 찾아보면서 공부하고, 문자열을 쪼개 파싱하는 능력을 기를 수 있을 것 같아 향후 보완해볼 예정입니다.
  • 모든 Document 정보에 대한 것을 한번에 랜더링해놓기 보다는 onclick 이벤트가 발생하였을 때에만 하위 document 정보를 DOM에 그려내는 방식으로 List 구조를 만들어 보았습니다. 클릭할 때마다 DOM을 그려내는 것이 올바른 설계일지 궁금합니다.
  • 컴포넌트에 의존하기보다는 각각의 페이지와 컴포넌트에서 자신의 역할만을 할 수 있도록 state를 관리하고자 하였습니다. 이러한 부분에 대한 구현이 잘 이루어진 편인지 궁금합니다.

@suyeon1218
Copy link

suyeon1218 commented Jul 10, 2023

유진님 구조도 그리시는 거 넘 깔끔해요!! 어떤 state 를 가지고 있는지도 적어주셔서 코드 리뷰 하는데 많은 도움이 됩니다 👍 마음에 들때까지 작업하시는 게 쉬운 일이 아니셧을 텐데 끝까지 노력하시고 대단하십니다...! 이번 과제도 넘 수고 많으셨어요!

@@ -0,0 +1,30 @@
import { EditorPage } from "./EditorPage.js";

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.

헉 감사합니다 !! 🥹


$target.appendChild($app);

this.route = () => {

Choose a reason for hiding this comment

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

EditorPage 를 먼저 선언해놓고 route 할 때마다 setState 를 바꿔주는 방식으로 구현하셨군요...! 이 방법으로 하니까 바로바로 읽히네요...!!

Copy link
Author

Choose a reason for hiding this comment

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

ㅎㅎ 맞습니다! EditorPage에서 url을 가져와 그때그때 문서의 id를 수정해 주는 방식으로 state를 만들어보려고 하였습니다!

@@ -0,0 +1,21 @@
const DEFAULT_ENDPOINT = 'https://kdt-frontend.programmers.co.kr'

export const request = async(url='', options) => {

Choose a reason for hiding this comment

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

request 부분도 하나의 함수로 만들어서 쓰셨네요! 파라미터를 헷갈릴 일이 없어서 좋은 것 같습니다! 더불어 제 코드는 파라미터를 알 수 없는 단점이 있어 불편하지 않았나... 하고 반성하게 되네요 ㅠ_ㅠ~!

src/router.js Outdated
}
})
}
export const push = (nextUrl) => {

Choose a reason for hiding this comment

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

push 라는 변수명이 라우터 체인지를 일으킨다고 알기는 조금 어려울 것 같아요! 조금 더 명시적으로 변수명을 지어야 할 것 같습니다...!

Copy link
Author

Choose a reason for hiding this comment

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

확인했습니다!!! 🫡 정말 네이밍이 세�상 젤 어려워요 ㅠㅠ 흑흑

push(`/documents/${id}`);
}
})
createBtn.render();

Choose a reason for hiding this comment

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

DocumentCreate 와 DocumentList 컴포넌트 내에서 각각 render 를 시키지 않고 상위 컴포넌트인 DocumentPage 에서 접근하여 render 를 시켜주시는 이유가 있으실까요!

Copy link
Author

Choose a reason for hiding this comment

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

헉...그런 방법이 있었군요 머리를 탁 치고 갑니다 수연님....

push(`/documents/${id}`);
}
})
documentList.render()

Choose a reason for hiding this comment

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

$documentPage.appendChild($documentList);
createBtn.render();
documentList.render();

중간에 있는 render 와 appendChild 도 아래로 빼내서 한 곳에 모아서 써주면 조금 더 읽기 편할 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

참고하여 수정하도록 하겠습니다! 감사합니다!

const documentList = new DocumentList({
$target: $parentNode,
data: [data],
depth: this.state.depth + 1,

Choose a reason for hiding this comment

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

depth 도 initialState 안에 포함돼 있는 프로퍼티 아닌가요...! 호출 시 5개의 파라미터를 전달해주게 됩니다..!

Copy link
Author

Choose a reason for hiding this comment

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

헉!! 그러네요!!! 이부분을 꼼꼼하게 확인못했습니다!! 수정해두겠습니다~~

$target: $editorPage,
initialState: this.state.post
})
childLink.render();

Choose a reason for hiding this comment

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

EditorPage 에서도 ChildDocument 를 render 해주게 되고, ChildDocument 컴포넌트 내에서도 스스로 render 를 호출해줍니다!
그리고 setState 로 다시 한번 render 가 일어나게 되면서 총 4번의 렌더링이 일어나고 있어요! 렌더링을 줄이는 게 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

헉 쓸데없이 랜더링을 한번 더 해주고 있었네요! 고쳐보겠습니다!


const documentDelete = new DocumentDelete({$target: $editorPage, id : this.state.id})
this.render = () => {
$target.appendChild($editorPage);

Choose a reason for hiding this comment

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

다른 곳에선 appendChild 를 별도의 render 함수에 담지 않고 실행해주고 있는데 여기선 this.render 함수 안에서 appendChild 함수 하나만을 실행시키신 이유가 있으실까요?!

Copy link
Author

Choose a reason for hiding this comment

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

사실 이 코드도 일관성이 잘 지켜지지 않은 사례입니다... 어떨 때는 render() 함수에 넣고 어떨 때는 컴포넌트가 생성될 때 DOM요소를 추가하도록 만들게 되었네요! 이 부분은 일관성을 지켜 수정해보도록 하겠습니다!

아무래도 render를 여러번 하게 될 수도 있으니 해당 함수에서 분리해내는 것이 더 좋을 것 같아 보이네요 ㅎㅎ

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.

이벤트 처리(stopImmediatePropagation, stopPropagation)도 배우고 갑니다! 거의 고정 멘트인듯 하지만 항상 많이 배웁니다. 노션 클로닝 수고하셨습니다!

Comment on lines +30 to +33
if ($closeBtn.classList.contains('closeBtn')){
const modal = document.querySelector('.modal');
modal.remove()
}

Choose a reason for hiding this comment

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

하위 페이지를 생성할 때 입력하는 부분에 클릭을 하게 되었는데 오류가 발생했습니다. $closeBtn의 존재에 대한 조건도 추가하면 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

현재 클릭 이벤트가 $closeBtn 한정으로 일어나지 않는 것으로 보이네요.
이에 대해서 가드할 수 있는 코드를 추가해 넣도록 하겠습니다. 감사합니다!

Comment on lines +57 to +68
if(this.state.isOpen){
while(e.target.querySelector('div')){
e.target.classList.remove("open");
const $removeTarget = e.target.querySelector('div')
e.target.removeChild($removeTarget);
}
this.setState({
...this.state,
isOpen: !this.state.isOpen,
})
return
}

Choose a reason for hiding this comment

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

의도와는 조금 다른 방식으로 동작하는 것 같습니다. 루트 컴포넌트에서 하위 컴포넌트를 보여줄 때에는 정상적으로 동작하지만 하위 컴포넌트의 하위 컴포넌트를 보여줄 때에는 이미 isOpen이 true인 상태여서 두 번 클릭해야 합니다. isOpen을 각각 적용 시키는 방법은 어떨지 생각해봤습니다!

Copy link
Author

Choose a reason for hiding this comment

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

각각의 컴포넌트별로 isOpen의 state를 관리할 수 있도록 리팩토링해보도록 하겠습니다! 감사합니다 🫡

Comment on lines 75 to 83
if(childrenData[0].length > 0){
this.setState({
parent: this.state.depth === 0 ? parseInt(id) : parseInt(this.state.parent),
selectedNode: parseInt(id),
isOpen: !this.state.isOpen,
depth: this.state.depth + 1
})
this.onSelect(childrenData[0], $li)
}

Choose a reason for hiding this comment

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

하위 컴포넌트가 있는 하위 컴포넌트를 열고 닫을 때 마다 depth가 증가합니다! 위에서 depth에 따라 margin을 다르게 주었기 때문에 계속 옆으로 밀려나는 듯 합니다!

Copy link
Author

Choose a reason for hiding this comment

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

헉!! 그런 에러가 존재하는군요, 열고 닫을 때마다 isOpen 상태가 true일때는 depth가 늘어나지 않도록 수정해야겠습니다. 꼼꼼한 리뷰 감사드립니다!

Copy link

@perfumeplaylist perfumeplaylist left a comment

Choose a reason for hiding this comment

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

##느낀점

  1. 컴포넌트 특징을 최대한 지킬려고 하는게 코드에서 느껴져서 보는데 재미있구 제가 못했던걸 해내셔서 배우면서 봤습니다!
    2.스타일도 많이 신경쓰신 모습을 보고 반성하게되었습니다ㅠㅠ
    3.Document부분에서는 코드 일관성을 잘 지켜서서 술술 읽혔는데 editor 부분에서는 코드 일관성이 지켜지지 않아서 살짝 아쉬웠습니다ㅠㅠㅠㅠ

<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<link rel="icon" href="asset/notion.png"/>

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.

ㅎㅎ 감사합니다!

Comment on lines 10 to 12
this.state = {
id : id,
}

Choose a reason for hiding this comment

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

한줄로 줄일 수 있을것 같습니다!
this.state = { id }

Copy link
Author

Choose a reason for hiding this comment

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

앗!! 그렇네요 ㅎㅎ 감사합니다

await deleteDocuments(this.state.id)
alert("삭제 완료");
push('/');
location.reload();

Choose a reason for hiding this comment

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

오 reload 메소드를 통해서 바로 reload하는게 유저 입장에서 좋은것 같습니다! 하나 배워갑니다! 👍

Copy link
Author

Choose a reason for hiding this comment

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

사실 state가 변경되는 것을 바로바로 볼 수 있도록 해주고 싶은데, 그 방법이 생각이 잘 나지 않아서 강제로 reload를 하게 되었어요
새로고침을 하게 되면 UI가 깜빡이기 때문에 유저 입장에서는 좋은 방법은 아닌 것 같아서 개인적으로는 살짝 구현에 있어 아쉬운 부분이라고 생각됩니다! 😂

Comment on lines 20 to 25
if(data.length === 0){
const $haveNothing = document.createElement('div');
$haveNothing.classList.add('nothing')
$haveNothing.textContent = '하위 페이지 없음'
$li.append($haveNothing)
}

Choose a reason for hiding this comment

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

자식이 없다면 바로 return으로 끝내주는게 좋아보입니다!

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 +30 to +37
const fetchDocuments = async() => {
const documentData = await getDocuments();
this.setState({
...this.state,
documentData
})
this.render()
}

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.

감사합니다!!

Comment on lines 46 to 47
documentDelete.setState({id: this.state.id})
childLink.setState({data: this.state.post})

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.

넵!!! 아무래도 validation을 검증할 수 있는 유틸함수를 따로 파야겠군요. 흐흐

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부분만 조금 다듬는다면 좋은 노션클로닝이 될 것 같습니다:)

@@ -0,0 +1,63 @@
.documentPage {
background-color: #F7F7F5;
min-width: 230px;

Choose a reason for hiding this comment

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

documentPage의 크기가 처음 기존화면과 클릭하고 난 뒤의 크기가 다른 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

아무래도 min-width로 생성하다 보니 리스트의 padding값에 따라서 더 커지는 것 같더라구요... 그냥 고정된 너비인 width로 수정해야겠습니다 😂

position:relative;
width:100%;
}
.createDoc{

Choose a reason for hiding this comment

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

li태그와 그의 형제인 btn의 높이가 많이 차이가 나는 것 같습니다!

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

@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 정리까지 정말 감동이군요!!

피드백

  • 라우터 파일 분리해서 관리하는 선택 좋습니다 👍
  • API 분리 시도 아주 좋아요. 조금은 아쉬운 면이 있지만 그래도 좋은 방향의 구현이네요

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

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

  • 이미 대체할 수 있는 Class가 등장했는데 굳이 생성자함수를 사용해야했을까?
  • 이렇게 될 경우 메서드 메모리가 어떻게 될까?

이외에 중요한 포인트는 코멘트 리뷰 남겨놨습니다.

Q&A

Q1. Rich한 에디터를 생성하려고 하였으나 (노션처럼 ##, #를 입력하면 자동으로 마크다운 형식으로 변환) 해당 기능을 구현하는 데 아직 공부가 미진한 듯 하여 완성하지 못했습니다. ㅠㅠ velog가 어떻게 만들어졌는지 래퍼런스를 찾아보면서 공부하고, 문자열을 쪼개 파싱하는 능력을 기를 수 있을 것 같아 향후 보완해볼 예정입니다.

아마 쉽지 않으실거에용
에디터를 만드는 개발팀이 따로 있는 경우도 있으니까요?
그래도 많은 학습이 되실테니 이것저것 시도해보세요 😄

Q2. 모든 Document 정보에 대한 것을 한번에 랜더링해놓기 보다는 onclick 이벤트가 발생하였을 때에만 하위 document 정보를 DOM에 그려내는 방식으로 List 구조를 만들어 보았습니다. 클릭할 때마다 DOM을 그려내는 것이 올바른 설계일지 궁금합니다.

코드 자체는 굉장히 지저분해진게 아쉽습니다 😢
하지만 굉장히 좋은 방향의 구현 방법이라 칭찬드립니다.
이런 시도 많이해주세요!

Q3. 컴포넌트에 의존하기보다는 각각의 페이지와 컴포넌트에서 자신의 역할만을 할 수 있도록 state를 관리하고자 하였습니다. 이러한 부분에 대한 구현이 잘 이루어진 편인지 궁금합니다.

음 아직은 아쉬운 부분이 정말 많습니다.
상태를 사용할때 아닐때 구분이 애매할때가 있고요.
상태를 CRUD하는 방법도 어색할때가 많아요.

하지만 지금처럼 계속 고민하고 시도해보시면 충분히 좋아지실 것 같네요 👍

@@ -0,0 +1,18 @@
<!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 class ="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.

😢

Copy link
Author

Choose a reason for hiding this comment

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

html파일까지 미처 신경 못썼네요!! 수정하도록 하겠습니다 🫡


export default function App ($target) {
const $app = document.createElement('div');
$app.className = 'mainApp'

Choose a reason for hiding this comment

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

다음부터는 표준도 참고해보세요 ㅎㅎ
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList

Copy link
Author

Choose a reason for hiding this comment

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

classList가 표준이군요! DOM의 클래스를 관리할 때 클래스 이름이 하나만 존재한다면 className으로 관리하는 것이 편리할 수 있으나, 여러개의 클래스를 가지고 있을때 이전에 어떤 class가 존재하든지 상관없이 교체해버리거나 중복된 class명을 덮어씌울 수 있다는 점도 있네요!! 다양한 메서드를 이용할 수 있는 classList를 이용해 보도록 해야겠습니다!! 메서드를 이용할 때 근거를 가지고 사용하는 것이 중요한 것 같은데 멘토님 덕분에 찾아보며 배우게 되는 것 같습니다 👍

src/App.js Outdated

this.route = () => {
const { pathname } = window.location;
if (pathname.indexOf('/documents/') === 0){

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.

if(pathname.includes('/documents/'))

이렇게 작성하면 더욱 간결하게 코드를 작성할 수 있을 것 같군요!! 이 메서드도 적극적으로 이용해도보록 하겠습니다 감사합니다!

src/App.js Outdated
}
}
this.render()
initRouter({onRoute : () => this.route()});

Choose a reason for hiding this comment

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

Suggested change
initRouter({onRoute : () => this.route()});
initRouter({onRoute : this.route});

이렇게는 안될까요?

Copy link
Author

Choose a reason for hiding this comment

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

지금 코드를 다시 보니까 콜백 형태로 넘겨줄 필요는 없는 것 같네요..ㅎㅎ

onClick = { () => func() }
onClick = {func}

옛날에 리액트를 공부하다가 이런 질문을 본 적이 있는데 항상 어떤 차이인지 궁금했었습니다!
제가 이해하기로는, onClick 의 props로 func 함수를 넘겨줄 때, 첫번째 경우는 바로 func()을 실행해야 하는 경우인 것이고, 두번째 경우는 func를 단순이 인자로 넘겨지는 것으로, onClick에서 추후 제가 현재 작성한 initRouter 처럼 함수 내부에서 알아서 다시 호출해주는 것의 차이라고 이해하였는데, 제가 이해한 부분이 맞는 것인지 궁금합니다!

Choose a reason for hiding this comment

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

ㅎㅎ 이것은 리액트와 조금 다른 결로 생각하시는게 좋아요
다만 말씀하신게 다 맞습니다!
공부 잘하셨네용

$li.classList.add('open')
if(childrenData[0].length > 0){
this.setState({
parent: this.state.depth === 0 ? parseInt(id) : parseInt(this.state.parent),

Choose a reason for hiding this comment

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

parseInt() 사용시 radix는 꼭 넣어주셔야합니다

Copy link
Author

Choose a reason for hiding this comment

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

지금까지 습관적으로 parseInt를 사용시에 radix를 넣어주지 않고 있었네요!! (당연히 10진수라고 생각해서..)
앞으로 radix를 넣는 것을 습관화하도록 하겠습니다!

$editor.className = 'editor'
$target.appendChild($editor);
this.state = initialState;
let isinitialize = false

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.

isinitialize는 한번 유효성을 체크하고 말 변수라고 생각하여 상태에 넣지 않았습니다!
상태가 계속하여 변화하는 것이나, 외부로부터 받아오게 되는 변수들을 state에 넣어서 관리해야 한다는 생각을 가지고 있었는데, 사실 로컬에서 관리하는 state이기 때문에 해당 변수도 상태관리해도 좋을 것 같다는 생각이 듭니다.!!

Choose a reason for hiding this comment

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

사실 isinitialize라는 상태 자체가 존재하면 안된다고 생각합니다 ㅎㅎ

this.render()
}

this.render = () => {

Choose a reason for hiding this comment

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

render() 내부에서 하는 일이 정말 너무!! 많아요
이건 꼭 분리하시는게 좋겠습니다

Copy link
Author

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.

언행일치한 닉네임을 작성해보시죵ㅎㅎ

export function EditorPage($target) {
const $editorPage = document.createElement('div');
$editorPage.className = 'EditorPage'
let timer = null

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.

EditorPage라는 컴포넌트의 이름을 가지고 있으니, state에는 글의 수정과 관련된 내용으로 상태관리를 해야 한다고 생각했습니다.
timer는 글의 수정, 삭제, 작성과 관련된 로직이 아니고, 해당 컴포넌트의 이벤트를 더욱 효율적으로 관리해줄 수 있는 변수라고 생각하여 상태에 포함하지 않았던 것 같습니다.
상태관리에 포함될 수 있는 것은 컴포넌트의 로직을 담당하는 상태들이 포함될 수 있다는 기준으로 작성하게 되었습니다!

Copy link
Author

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.

후후.. 어렵죠?
상태냐 아니냐 그것을 나누는 나만의 기준이라도 있는게 좋겠어요.
React로 치면 이것은 useState()가 아닌 useRef()로도 사용할 수 있거든요

Comment on lines +42 to +45
editor.setState(this.state.post || {
title:'',
content:''
});

Choose a reason for hiding this comment

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

기본 값이 반복될거라면 editor.setState() 내부에서 해주는게 좋지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

헉 애초에 editor 내부에서 세팅해주는 것이 더 간결하게 작성할 수 있을 것 같네요!! 감사합니다!

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