[Mission5/김다은] Project_Notion_VanillaJs 과제 #54

GBAJS754 · 2023-07-06T15:57:44Z

베포 링크

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

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

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

뒤로가기를 눌렀을때 해당 url이 삭제된 페이지라면 api 호출 오류가 뜹니다. 이를 방지하고자 api get요청을 통해 전체 List를 다시 가져와 해당 id가 있는지 탐색한뒤 없을 경우 "/"페이지로 오류없이 넘어가고자 했습니다. 하지만 결국 api요청이 늘어나서..😭 제가 생각한 방법보다 더 좋은 방법이 없을까요?ㅎㅎ;; 아무래도 api요청이 적을수록 좋은거겠죠..?
List가 다시 렌더링 되어도 토글된 상태를 저장하고자 로컬스토리를 사용했습니다. 하지만 로컬스토리지는 사용자도 볼수있는 부분이라서 어떤 값이 들어가는게 옳은지? 유효한 것인지 구분이 잘 안갑니다. 로컬스토리지를 어디까지 사용하는것이 좋을까요..? 토글된 상태를 저장하고자 로컬 스토리를 사용하는것이 옳은 걸까용? 아니면 각 문서마다 토글된 상태를 가지고 있는 방법은 없을까용? 혹시 다른 좋은 방법이 있다면 알려주시면 감사하겠습니당!🧐

Eunseo-jo

코드가 간결해서 읽기 편했습니당☺️ 과제하느라 고생 많으셨습니당✨

Eunseo-jo · 2023-07-09T15:24:06Z

src/utils/router.js

+export const updateDocumentTitle= (onUpdate)=>{
+    window.addEventListener(TITLE_UPDATE_EVENT_NAME, ()=>{
+        onUpdate()
+    })
+}
+
+export const update = ()=>{
+    window.dispatchEvent(new CustomEvent('title-update',{
+    }))
+}


왕 커스텀이벤트로 제목을 업데이트하는 방법 좋네요👍👍

Eunseo-jo · 2023-07-09T15:27:30Z

src/utils/storage.js

+    toggle: id => {
+        const getOpenedDocuments = getItem(OPENED_DOCUMENTS, []);
+        if (getOpenedDocuments.includes(id)) {
+          const removeOpenedDocuments = getOpenedDocuments.findIndex(
+            documentId => documentId === id,
+          );
+          if (removeOpenedDocuments !== -1)
+          getOpenedDocuments.splice(removeOpenedDocuments, 1);
+          setItem(OPENED_DOCUMENTS, [...getOpenedDocuments]);
+        } else {
+          setItem(OPENED_DOCUMENTS, [...getOpenedDocuments, id]);
+        }
+      },


toggle이 add랑 delete 부분이랑 중복되는 코드가 있는뎅 toggle로 한번에 처리하면 좋을꺼 같습니당 :)

bbearcookie

노션 클로닝 하시느라 고생하셨습니다~
전체적으로 코드가 깔끔해서 잘 읽혔어요!

bbearcookie · 2023-07-10T05:26:30Z

src/components/CreateLists.js

+    </ul>`
+    }
+    ).join("")
+}


DocumentLists 컴포넌트의 렌더링 로직에서 그려야 할 HTML 템플릿을 따로 함수로 구현해주신 점 좋네요!
다만 파일이 src/components 디렉토리에 존재하고 있어서 이것이 컴포넌트라는 생각이 들 수도 있을 것 같아요!

사실 컴포넌트라기보다는 들어온 인자를 가지고 가공해서 문자열을 반환하는 순수 함수의 형태인 것 같아서
이 함수를 DocumentLists.js 파일 내부에 존재하도록 하거나 혹은 template/createList.js 같은 느낌으로 이사하는건 어떨까요?

bbearcookie · 2023-07-10T05:32:16Z

src/utils/api.js

+        console.log(e.message)
+        push("/")
+    }
+}


서버에 요청을 보내는 일반화된 함수 request 는 api.js 에 존재하고,
실제로 구체화된 요청을 보내는 함수는 request.js 에 존재하는 형태인 것 같습니다!

request 함수는 request.js 에 작성하고, 구체화된 요청을 하는 함수는 api.js 에 작성하는 형태가 더 좋지 않을지 궁금합니다!

bbearcookie · 2023-07-10T05:33:10Z

src/utils/requset.js

파일 이름이 requset.js 로 오타가 있었던 것 같습니다!

suehdn · 2023-07-10T13:08:50Z

안녕하세요 다은님. 코드 잘 보고 배워갑니다!!
과제하시느라 고생 많으셨어요 ☺️

hyeonjun-L

너무 깔끔한 코드 잘 봤습니다! 다은님은 정말 깔끔하게 작성하시는 능력이 엄청나신 것 같아요! 잘 보고 많이 배우고 갑니다 👍

sairion · 2023-07-12T23:17:46Z

뒤로가기를 눌렀을때 해당 url이 삭제된 페이지라면 api 호출 오류가 뜹니다. 이를 방지하고자 api get요청을 통해 전체 List를 다시 가져와 해당 id가 있는지 탐색한뒤 없을 경우 "/"페이지로 오류없이 넘어가고자 했습니다. 하지만 결국 api요청이 늘어나서..😭 제가 생각한 방법보다 더 좋은 방법이 없을까요?ㅎㅎ;; 아무래도 api요청이 적을수록 좋은거겠죠..?

삭제된 페이지라면 요청 시 HTTP code 404가 반환되고, 그에 따른 처리를 하는 것이 맞습니다. 그 방식을 우회해서 list를 전체를 다시 가져오는 것은 효율성 측면에서도 그렇고, 일반적이지 않은 접근 방식인 것 같습니다. ^^

List가 다시 렌더링 되어도 토글된 상태를 저장하고자 로컬스토리를 사용했습니다. 하지만 로컬스토리지는 사용자도 볼수있는 부분이라서 어떤 값이 들어가는게 옳은지? 유효한 것인지 구분이 잘 안갑니다. 로컬스토리지를 어디까지 사용하는것이 좋을까요..? 토글된 상태를 저장하고자 로컬 스토리를 사용하는것이 옳은 걸까용? 아니면 각 문서마다 토글된 상태를 가지고 있는 방법은 없을까용? 혹시 다른 좋은 방법이 있다면 알려주시면 감사하겠습니당!🧐

토글된 상태를 저장해도 문제가 있는 것은 아닙니다. 어짜피 이미 유저에게 보여주고 있는 정보는 로컬스토리지에 저장해도 크게 다른 것은 없다고 볼 수 있겠죠~ 좀 더 좋은 방법은 (아마 현재 시점에서 서버 구현을 바꾸기는 어려울 것이므로 구현할 수 있을지는 모르겠지만) 토글이 되어있는지 여부를 서버 측에서 저장하고 있는 것이 아닐까 합니다. 그 이전에, 토글되었는지 여부를 저장하는 것이 좋은지에 대해 생각해 보면 좋을 것 같지만요.

sairion · 2023-07-13T04:27:52Z

src/App.js

+        $target.innerHTML = ""
+        if(pathname === "/"){
+            documentPage.render()
+        }else if(pathname.indexOf("/documents/") === 0){
+            const [,,documentId] = pathname.split("/")
+            documentPage.render()
+            editorpage.setState({documentId})
+        }else{
+            $target.innerHTML = ""
+            documentPage.render()
+        }


Suggested change

$target.innerHTML = ""

if(pathname === "/"){

documentPage.render()

}else if(pathname.indexOf("/documents/") === 0){

const [,,documentId] = pathname.split("/")

documentPage.render()

editorpage.setState({documentId})

}else{

$target.innerHTML = ""

documentPage.render()

}

$target.innerHTML = ""

documentPage.render()

if(pathname.indexOf("/documents/") === 0){

const [,,documentId] = pathname.split("/")

editorpage.setState({documentId})

}

공통된 부분이 많아 이렇게 줄여볼 수 있겠네요~

sairion · 2023-07-13T04:34:26Z

src/components/CreateLists.js

+        return`<ul data-id="${id}">
+        <div class="document__title">
+            <span>
+                <button class="toggle__button">></button>


Suggested change

<button class="toggle__button">></button>

<button class="toggle__button">></button>

HTML에서는 마크업 문제가 있을 수 있으니 > < 를 이용하길 권장합니다.

sairion · 2023-07-13T16:08:25Z

src/pages/EditorPage.js

+        $target : $page,
+        initialState :{
+            title : "",
+            content :""},


initialState가 인자로 특별히 필요한지 잘 모르겠습니다.

sairion · 2023-07-13T16:09:49Z

src/components/Editor.js

+        if(!init){ 
+            $editor.innerHTML=`
+            <input class="title" style="width:800px;height:70px;" value="${this.state.title}">
+            <textarea class="content" style="width:800px;height:500px;">${this.state.content}</textarea>


굳이 인라인 스타일을 쓰지 않아도 될 것 같습니다.

suehdn · 2023-07-10T13:28:53Z

src/components/Header.js

+export default function Header({$target}){
+    const $header = document.createElement("h2")
+    $header.innerHTML = "김다은의 Notion"
+    $target.appendChild($header)
+}


처음에 Header 컴포넌트를 보고 <header>태그를 사용하신 컴포넌트로 이해했습니다.
위치는 <header> 태그를 이용했을때와 같겠지만 태그를 이용하는 편이 이해하기 좀 더 수월할 수도 있지 않을까요? ☺️

suehdn · 2023-07-10T13:30:27Z

src/components/Footer.js

+export default function Footer({$target, onClick}){
+    const $button = document.createElement("button")
+    $target.appendChild($button)
+    $button.textContent = "페이지 추가"
+    $button.className = "footer"
+
+    document.querySelector(".footer").addEventListener("click", (e)=>{
+        onClick()
+    })
+}


Header와 같은 의미로 <footer> 태그를 이용하시는 편이 위치를 이해하기 좋을 것 같다는 생각이 듭니다.☺️

GBAJS754 added 6 commits July 4, 2023 15:53

Feat : 에디터 기능 구현

47fb267

Feat: toggle기능 추가

747a4f3

Fix : 토글기능 수정

c7c32e1

Fix

2a27877

Fix : 뒤로가기 Api 오류 수정

4d140db

fix

0fbf903

GBAJS754 requested a review from sairion July 7, 2023 03:47

GBAJS754 added 재호팀 🔥 매운맛 과제 리뷰 요청 labels Jul 7, 2023

GBAJS754 self-assigned this Jul 7, 2023

GBAJS754 added the 피드백 요청 label Jul 7, 2023

Eunseo-jo reviewed Jul 9, 2023

View reviewed changes

bbearcookie reviewed Jul 10, 2023

View reviewed changes

hyeonjun-L reviewed Jul 10, 2023

View reviewed changes

sairion reviewed Jul 13, 2023

View reviewed changes

suehdn reviewed Jul 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mission5/김다은] Project_Notion_VanillaJs 과제 #54

[Mission5/김다은] Project_Notion_VanillaJs 과제 #54

GBAJS754 commented Jul 6, 2023

Eunseo-jo left a comment

Eunseo-jo Jul 9, 2023

Eunseo-jo Jul 9, 2023

bbearcookie left a comment

bbearcookie Jul 10, 2023

bbearcookie Jul 10, 2023

bbearcookie Jul 10, 2023

suehdn commented Jul 10, 2023 •

edited

Loading

hyeonjun-L left a comment

sairion commented Jul 12, 2023

sairion Jul 13, 2023

sairion Jul 13, 2023

sairion Jul 13, 2023

sairion Jul 13, 2023

suehdn Jul 10, 2023

suehdn Jul 10, 2023

	<button class="toggle__button">></button>
	<button class="toggle__button">></button>

[Mission5/김다은] Project_Notion_VanillaJs 과제 #54

Are you sure you want to change the base?

[Mission5/김다은] Project_Notion_VanillaJs 과제 #54

Conversation

GBAJS754 commented Jul 6, 2023

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

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

Eunseo-jo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbearcookie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suehdn commented Jul 10, 2023 • edited Loading

hyeonjun-L left a comment

Choose a reason for hiding this comment

sairion commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suehdn commented Jul 10, 2023 •

edited

Loading