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 과제 #38

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

Conversation

jgjgill
Copy link
Member

@jgjgill jgjgill commented Jul 6, 2023

📌 과제 설명

  • 바닐라 JS만을 이용해 노션을 클로닝합니다.

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

실행 방법

yarn
yarn dev

배포 링크

내가 생각하는 구조도

notion

✅ PR 포인트 & 궁금한 점

안녕하세요! 이번 과제를 진행하면서 너무 생각이 많아지게 되었는데요. 그러다보니 생각의 생각의 꼬리를 물게 되었던 것 같습니다.. 😂
그래서 코드가 일관적이지 않고 너무 복잡하게 구성되어서 마음이 아프네요..
처음 목표는 추상화하기, 상태 관리 연습해보기 였는데 실패했습니다..
아직 이 부분에 대한 연습과 역량이 부족하다는 것을 이번 과제를 진행하면서 뼈져리게 느꼈습니다..😇

그래도 과제는 진행해야 하니 어찌저찌 돌아가는 코드라도 구성했습니다. (기능을 추가하면서 점점 코드가 복잡해지는 것을 보니 힘들었습니다..)
투두리스트와 비교했을 때 왜 어려워졌을까 고민했는데 서버와 알고리즘(재귀)가 적용되어서 그런 것 같습니다.
그래서 설계도에서 RecurDocumentEditor, RecurDocumentList 이 부분에서 설계가 잘못되어 코드가 난잡해졌다는 생각을 했습니다.

코드가 너무 복잡한 것 같아서 죄송한 마음이 큽니다.. 추상화를 높여서 깔끔하고 직관적인 코드를 짜고 싶었는데 참 힘드네요 이게..
이번 과제는 노션과의 대결에서 졌다는 생각을 하게 되었습니다.. 추상화에 대한 학습을 계속 해나가면서 반드시 극복하겠습니다..
감사합니다.. 🙇‍♂️

라우팅 관련 작업
api 설정
추가 버튼 누르면 바로 생성하는 방식
문서 리스트 옆에 위치 변경
AddButton 텍스트 추가
DocumentItem 자식 문서 추가, 문서 삭제 관련 작업
재귀 관련 렌더링되는 함수 template 폴더로 구분
DocumentEditor로 변경
api 관련 작업 분리
new 버튼 클릭시 상태 변경 작업
삭제 버튼 클릭시 상태 변경 작업
라우팅 이동시 남아있는 페이지 제거
전체적인 스타일 작업
자잘한 코드 수정(findAllDocument 위치 수정, RecurDocumentList 객체로 매개변수 받는 형태로 변경
자식 문서있는 문서 삭제시 루트로 이동 후 id순으로 오름차순 정렬
주석 제거
이름 변경
코드 위치 변경
@bomi8489
Copy link

bomi8489 commented Jul 9, 2023

종길님 이번 과제 수행하시느라 너무 고생 많으셨습니다!!🤭 항상 기존의 요구사항을 넘어 확장성을 고려하시는 모습 저도 배워야 할 것 같네요...


return searchList;
};
}
Copy link

Choose a reason for hiding this comment

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

trie 실습 했던걸 활용해서 검색 기능 만드신게 정말 인상 깊습니다... 👍👍

containerElement.classList.toggle("toggle");
return;
}

Copy link

Choose a reason for hiding this comment

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

저는 하위 리스트가 디폴트로 접혀있고 토글을 통해 열어보는 방식이어서 document가 변경될때마다 재렌더링 되어서 다시 접히는 문제를 겪었는데 디폴트로 열어두는 방법도 있었네요! 👍👍 근데 하위 document가 많아지면 사이드바가 자칫 지저분해 보일 수도 있는데 이부분에 대해서 어떻게 생각하세요??

Copy link
Member Author

Choose a reason for hiding this comment

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

맞네요!! 기범님이 말한 경우를 생각하면 디폴트로 접혀있는 경우도 더 나을 것 같아요!!
디테일한 부분 말씀해주셔서 감사합니다..! 🙇‍♂️

Copy link

@colorkite10 colorkite10 left a comment

Choose a reason for hiding this comment

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

종길님, 이번 노션 클로닝 끝마치시느라 고생 많으셨습니다! 🕺🕺

이번 PR을 읽으면서 종길님이 어떤 부분을 깊게 고민하신지 느껴졌고, 코드에서도 추상화를 위해 노력하신 부분이 인상 깊었어요! 개인적으로 document.js을 이해하는 데에 제일 공을 만히 들였습니다.

notFound 오류 처리 하신 것도 배울 점이라고 생각했습니다!
또한 문서 추가/삭제 버튼 위에 커서를 올리면 설명 텍스트가 뜨는 부분에서 사용자 경험도 고려하셨구나 하고 감탄했습니다! 홈 화면으로 이동할 수 있는 Jontion에도 설명해주는 텍스트가 있었다면 더 좋았을 것 같습니다. 다른 문서들을 구경하다가 다시 홈 화면으로 돌아가서 자동완성 기능을 구경하려고 했었는데 버튼을 못 찾았었거든요(뒤늦게 찾음)ㅠㅠ

기존에 데브코스에서 배웠던 내용들을 적극적으로 활용하셔서 종길님 코드를 읽으면서 다시 복습하게 되는 효과도 있었어요 ㅎㅎ

사실 코드를 완전히 이해하진 못했지만... 커밋 기록을 상세하게 남겨 주신 점도 좋았네요!

수고하셨습니다 😊😊😊

*/

export default function App({ appElement }) {
if (!new.target) return new App(...arguments);

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.

오 디테일 하시네요!! 👍

Copy link
Member Author

@jgjgill jgjgill Jul 13, 2023

Choose a reason for hiding this comment

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

저도 모든 컴포넌트에는 추가하지 못했습니다.. 😂

Copy link

@1eecan 1eecan left a comment

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 32 to 35
wrapperContainer.className = "wrapper-container";
leftContainerElement.className = "left-container";
rightContainerEleement.className = "right-container";
leftListElement.className = "left-list-container";
Copy link

Choose a reason for hiding this comment

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

클래스명을 이렇게도 줄 수 있군요! 이렇게 주면 어떤점이 좋다고 생각하시나요?! innerHTML을 안써도 되는 점일까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 요소는 따로 렌더링하기 위한 내부 요소들이 필요하지 않아서 className으로 준 것 같네요..!
비슷한 코드끼리 묶어두면 구분하기에도 용이하구요!! 😆

Comment on lines +1 to +4
export default function Tooltip({ text }) {
this.toggle = (targetElement) => {
targetElement.nextElementSibling.classList.toggle("toggle");
};
Copy link

Choose a reason for hiding this comment

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

p5 : Tooltip이라는게 hover시에 표시를 해주는 뜻이라는 걸 처음 알게 되었습니다! tooltip이라는 단어를 잘몰라서 그럴수도 있는데 toggle 보다는 hover가 조금 더 직관적으로 와닿는 느낌? 인것 같습니다..!

Copy link
Member Author

Choose a reason for hiding this comment

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

코드를 작성할 때는 classList.toggle로 되어 있어서 생각의 흐름이 저절로 toggle로 이름을 짓게 된 것 같네요..! 😂
저도 기능이나 용도는 hover가 더 맞는 것 같은데 사용할 때 옆에 toggle이 있어서 고민 좀 해보겠습니다..!

Copy link

Choose a reason for hiding this comment

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

처음에 코드에 debounce가 쓰이는 부분이 두 부분 밖에 없는데 함수로 뺄 이유가 있으셨을까? 라고 생각이 들어서 고민을 해봤는데, 한번에 timeout을 변경할 수 있다는 점에서 이득이 있을 수 있다는 생각이 들었는데 맞게 추측한 걸까요??🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

기존 코드대로 쓰면 timer를 선언하는 부분이 주변 코드들과 전혀 상관없이 겹치게 되는 것 같습니다..!
timer만 보면 어디에 쓰이는 코드인지 한 번에 이해하기 어려울 것 같다는 생각도 들고요.
debounce는 많이 쓰이는 코드여서 이왕 쓰는 김에 따로 분리해봤습니다..! 😆

@nayeon-hub
Copy link

종길님 노션과 대결하시느라 고생 많으셨습니다👍
코드를 보는데 파일들이 폴더로 잘 구분되어 있어서 코드를 찾기 수월했습니다. 폴더 구조도 중요하다는 것을 깨닫게 되네요! 파일 명 중에 궁금점이 있어요 domain이라는 파일 명은 어떤 걸 의미하나요?
추상화 하려 했던 노력들도 잘 느껴졌습니다. 컴포넌트들이 세부적으로 분리되어 있어서 컴포넌트 마다 기능들을 파악하기 쉬웠습니다

src/App.js Outdated
documentEditorComponent.render();
};

const layoutComponent = new Layout({ parentElement: leftContainerElement });

Choose a reason for hiding this comment

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

layout Component를 따로 분리하셨는데 의도가 궁금합니다! 다른 컴포넌트에서도 사용될 것을 염두해서 분리하신건가요?🤔

Copy link
Member Author

@jgjgill jgjgill Jul 13, 2023

Choose a reason for hiding this comment

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

노션 구조에 대해 계속 생각하면서 막상 변경되는 것은 컨텐츠 부분만 해당하더라구요.
그래서 처음에는 레이아웃 부분이 많을 것 같아서 이렇게 구성하게 된 것 같네요..! 🧐

import Tooltip from "./Tooltip.js";

export default function AddButton({
parentElement,
Copy link

@nayeon-hub nayeon-hub Jul 10, 2023

Choose a reason for hiding this comment

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

parentElement라는 변수명 이해하기 정말 쉽네요! 부모 element에 들어가는 컴포넌트들에게 target 대신 parentElement 변수명을 사용하니까 직관적이고 더 명확하게 의미를 알게되네요😮

Copy link
Member Author

Choose a reason for hiding this comment

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

근데 장단점이 있는 것 같습니다..! 😂
targetElement로 쓰이는 경우도 생길 것 같은데 계속 사용해보면서 고민할 것 같네요!!

Copy link
Member

@sukvvon sukvvon left a comment

Choose a reason for hiding this comment

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

종길님!! 과제 하시느라 고생하셨습니다!! 생각을 많이 하시면서 고생하신 모습이 코드에 보여서 굉장히 감명받았습니다. 전 요즘 종길님의 코드를 보면서 사고를 넓히는 것 같아서 너무 좋아요 ㅎㅎ 앞으로도 잘 부탁드립니다! 😁

Comment on lines +6 to +13
this.render = (content) => {
return `
<div class="tooltip">
${content}
<div class="tooltip-text">${text}</div>
</div>
`;
};
Copy link
Member

Choose a reason for hiding this comment

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

return을 사용하신 점이 인상적입니다!! 👍

const tooltipRemoveElement = new Tooltip({ text: "삭제" });

containerElement.addEventListener("click", (e) => {
if (Number(e.target.closest("li").id) !== documentData.id) return;
Copy link
Member

Choose a reason for hiding this comment

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

early return을 활용하신점이 인상적입니다!! 😁👍

Comment on lines +1 to +3
/**
* @description 현재 문서의 자식 문서
*/
Copy link
Member

Choose a reason for hiding this comment

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

👍👍👍

Copy link

@eastroots92 eastroots92 left a comment

Choose a reason for hiding this comment

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

와우 종길님 코드에서 고민을 많이 하셨다는게 느껴져서 좋았어요.

이미 PR 포인트에 적어두었던.
상태관리 연습해보기 이 부분이 잘 되지 않아 자연스레 추상화나 복잡하다 느껴지지 않았을까 해요.
옵저버 패턴 같이 상태 관리를 하지 않았음을 고려해 보았을 때 적절한 추상화를 했다고 생각 되었어요.
코드를 읽으면서 복잡하다고 생각되지도 않았구요.

그래서 별도의 코멘트를 달 것은 별로 없었는데.

소소하게 가독성 & 좋은 코드를 위해 고민해 보셨으면 하는 것으로는

  • 하나의 함수에는 하나의 목적(기능)만 하게 하자.
  • 역할과 책임을 명확하게 하고 다른 곳에서 위임하지 말자. (ex. fetch는 fetch 역할만 컴포넌트는 컴포넌트만)

였어요!! 이 부분에 대해서는 티타임 때 조금 더 이야기 나눠봐요!! 고생 많으셨어요~~!

*/

export default function App({ appElement }) {
if (!new.target) return new App(...arguments);

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 28 to 30
const leftContainerElement = document.createElement("div");
const rightContainerEleement = document.createElement("div");
const leftListElement = document.createElement("div");

Choose a reason for hiding this comment

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

TMI

저도 실제로 left와 right를 자주 사용하긴 하는데.
안드로이드 등에서는 컴포넌트를 만들 때 left, right 대신 start, end 라는 용어를 쓰더라구요.
그 이유가 무엇인지 아시나요?

바로 언어 호환 때문인데요! 중동어의 경우 시작이 오른쪽에서 왼쪽으로 흘러가기 때문에
왼쪽 정렬이라는 말을 쓰게 된다면 이게 시작점을 의미하는 것인지? 끝 점을 이야기 하는 것인지 혼선이 있을 수 있기 때문이에요!

여기서 strat, end를 쓰라는 뜻은 아니였고 멀티 랭기지를 챙기게 되면 이런 혼선이 있을 수 있다!! 정도로
그냥.. TMI 로 말씀 드려봐요.. ㅋㅋㅋ

다만 여기서는 left, right 보다는 sidebar, contents(or main) 등 시멘틱한 용어를 써서 구분을 지어도 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

오오..! 전혀 생각지도 못한 부분이였네요..! 😂
좋은 꿀팁 감사합니다!!


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

this.init = async () => {

Choose a reason for hiding this comment

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

이렇게 Init 함수를 만든 것 너무 좋네요~~ 👍

Comment on lines +136 to +145
if (pathname === PATH.HOME) {
trie.reset();
insertAllDocument(this.state, (title, id) => trie.insert(title, id));

homeComponent.render();
} else if (pathname.split("/")[1] === "documents") {
documentEditorComponent.render();
} else {
notFoundComponent.render();
}

Choose a reason for hiding this comment

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

P2;

추후 서비스가 확장된다면 다양한 url이 추가될 것 같은데 그때 else if 문이 반복된다면 코드를 파악하는데 오랜 시간을 써야할 것 같다는 생각이 들었어요.

switch 문을 써보는 것은 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

네넵..! 해당 부분은 변경이 필요해 보이네요!! 참고하겠습니다! 😆

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

Choose a reason for hiding this comment

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

오 이렇게 관심사 별로 fetch 함수들을 따로 빼는 것 너무 좋네요!! 👍

중복되는 값들은 별도의 상수로 관리하는 것도 너무 좋구요!! 👍👍


const API_END_POINT = "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.

P5;

url은 보통 https~~ 가 포함된 것을 의미하기 때문에 지금의 경우 path라고 정의하는 것이 더 적절할 것 같아요!
image

Copy link
Member Author

Choose a reason for hiding this comment

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

그렇군요!! 아무 생각없이 반복적으로 url이라는 단어를 사용하고 있었네요!!
이렇게 하나 또 배워갑니다..! 🧐

Comment on lines +16 to +20
if (res.ok) {
return await res.json();
}

throw new Error("API 에러가 발생했습니다!");

Choose a reason for hiding this comment

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

P3;

res.ok가 아닌 경우에는 어떻게 핸들링 하나요?
4xx번대 에러와 5xx번대 에러의 경우 각기 다르게 핸들링 해야할 것 같은데 여기서는 throw new Error("API 처리 중 이상 발생"); 로 퉁쳐지는 것 같아서요!

ask; 퀴즈

request 함수 관점에서
서버에서 정상적으로 응답이 온 경우!
특히 유저가 실수 해서 난 Error인 4xx번대 에러인 경우 이를 코드상 에러라고 보는 것이 맞을까요?
아니면 정상적으로 동작하는 것으로 보는게 좋을까요?
만약 정상이라고 본다면 어떻게 처리하는 것이 좋을까요?

Copy link
Member Author

@jgjgill jgjgill Jul 13, 2023

Choose a reason for hiding this comment

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

에러에 대해서 깊게 생각하지 않고 하나로 퉁쳤네요. 😂
말씀하신 것처럼 코드상 에러가 아닌 경우도 있어서 생각을 더 해봐야 되겠네요..
에러 핸들링과 관련해서도 코드를 짜보는 연습을 해보겠습니다..! 감사합니다!! 🙇‍♂️

Comment on lines 22 to 23
alert(err.message);
push(PATH.HOME);

Choose a reason for hiding this comment

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

P1;

request라는 함수의 역할을 생각해 보았을 때
딱 있는 그대로 데이터를 요청하고, 그에 맞는 응답을 return 하는 것이라고 생각돼요.

request 함수 이름으로는 error가 발생 헀을 때 UI상의 변경(alert가 뜬다던지, 특정 페이지로 리다이렉트 된다던지) 하는 것을 유추하기 힘들 뿐 아니라.
상황에 따른 유연한 대처가 쉽지 않아 좋지 않은 코드라고 생각돼요.
(만약 글쓰기 화면에서 글 작성 API를 요청하고 실패 했을 때 실패했다 안내와 함께 home으로 리다이렉트 된다면 매우 화가날 것 같아요.)

여기서는 딱 request 함수 이름에서 나타내는 하나의 역할에만 집중하고, 에러 핸들링(특히 UI)의 경우 UI 관련된 곳에서 처리하게 하는 것이 좋을 것 같은데 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

네넵..! 해당 코드를 다시 보니 당장의 편리함에 request에 강제적인 에러 처리를 한 것 같네요!! 😥
request는 데이터 요청과 응답에만 집중한다! UI와 관련된 에러 핸들링은 그와 관련된 곳에서 처리한다!
명심하겠습니다!!

@@ -0,0 +1,10 @@
export function debounce(func, timeout = 300) {

Choose a reason for hiding this comment

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

오 멋지네요! 👍


const API_END_POINT = "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 함수를 어떻게 말아서 쓰면 좋을까? 고민이 되신다면

axios 또는 ky 라이브러리는 어떻게 쓰라고 되어있는지?
왜 이렇게 쓰게끔 했는지?
만약 fetch 함수로 ky 또는 axios를 만들려면 어떻게 할 수 있을지?

쓰윽 고민해보시면 좋을 것 같아요!

TMI

  • axios는 fetch 이전의 예에에전 방식의 xmlhttprequest 방식을 쓰고 있어서 fetch와 같은 역할을 모두 지원하기 위해 번들 자체가 느리고 무겁지만 다양한 미들웨어를 제공하고 유니버셜하게 쓸 수 있어서 많이 사용되는 라이브러리에요.
  • ky의 경우 fetch 기반으로 되어있어서 매우 가볍고 편하지만 interceptor 와 같은 미들웨어를 지원하지 않아요.

Copy link
Member Author

Choose a reason for hiding this comment

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

네넵..! 해당 라이브러리 참고해보겠습니다!
감사합니다..! 🙇‍♂️

left, right 대신 sidebar, contents로 변경
사용하는 영역에서 에러 처리하도록 코드 변경
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.

7 participants