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

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

Conversation

sscoderati
Copy link
Member

@sscoderati sscoderati commented Jul 6, 2023

📌 과제 설명

Notion을 클론!! 해보는 프로젝트지만, 마감 직전에 살펴보니 조금 발전된 형태의 TodoList 같습니다...
배포는 했지만, 발견되거나 발견되지 않은 에러가 많은 듯 하네요..
https://sscoderati-notion-clone.netlify.app/

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

기본 요구 사항별 구현 상황

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

  • 기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.

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

    • 내부에서 문서를 선택해서 각 문서별 내용을 불러올 수는 있지만, 외부에서 해당 URL로 접속을 시도하면 404 에러가 납니다..
    • 7월 7일자 강의 보고 해결했습니다! (_redirects 파일 추가)

추가 구현 사항

  • 실제 Notion처럼 Title을 에디터에서 편집하면 좌측 문서 목록의 편집 중인 문서의 Title이 같이 편집됩니다.
  • 단일 Document 항목 좌측의 펼치기(>) 버튼을 클릭해서 1단계 depth의 하위 Document를 보이게 하거나, 감출 수 있습니다.
    • 단, 하위 Document를 추가한 직후에 해당 기능을 사용하려고 하면, 동작하지 않습니다...
  • 존재하지 않는 문서 Id로 접근하려고 하면 404 not found 대신 지정한 화면이 라우팅 됩니다.

✅ 피드백 반영사항

  • prettier formatting 적용
  • 역할이 잘 드러나지 않는 함수 재명명
  • Component 클래스 상속 시 render() 메소드 호출 강제화
  • debounce 유틸함수 처리

✅ PR 포인트 & 궁금한 점

각 컴포넌트의 render 부에서 너무 많은 것들을 담당하고 있어서 외부 함수로 분리하고 싶은데, 상태 처리나 비동기 함수의 호출 관계가 복잡해져서 끝내 분리하지 못했습니다. 혹시 관련하여 참고하면서 공부할 만한 글이 있을까요..?

Copy link

@off-programmers off-programmers left a comment

Choose a reason for hiding this comment

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

image
리스트를 넘어가는 경우도 있네요~

image
하위 레이어가 접히게 버튼을 눌러도 반응하지 않는 경우가 있네요~

리뷰 완료입니다~

.eslintrc.js Outdated
"browser": true,
"es2021": true
},
"extends": "eslint:recommended",

Choose a reason for hiding this comment

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

prettier 규칙도 같이 적용되게 해보시죠~

Copy link
Member Author

Choose a reason for hiding this comment

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

9ab6c73 적용 완료했습니다!

index.html Outdated
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">

Choose a reason for hiding this comment

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

meta 설정에 있어 어떤 값들이 들어갈 수 있는지 이참에 찾아보면 좋을것 같네요~

Copy link
Member Author

Choose a reason for hiding this comment

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

viewport의 개념과 content 속성에 들어갈 수 있는 width=device-width, initial-scale, minimum-scale, maximum-scale에 대해 알게 되어 적용하였습니다! 88855af 다음 과제부터는 기본적으로 적용해보겠습니다!

@@ -0,0 +1,12 @@
import { PostRouter } from "./PostRouter";

export const PostChanger = (appEl) => {

Choose a reason for hiding this comment

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

메서드 이름에서 뭔가 어떤 역할이다라는게 딱 떠오르지는 않는것 같은데..이름을 바꿔주면 좋을것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

9ab6c73 editorViewSwitcher로 이름 수정하였습니다!

@@ -0,0 +1,12 @@
import Editor from "../components/Editor";

export const PostRouter = (appEl, introEl) => {

Choose a reason for hiding this comment

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

요기도 컴포넌트라기보다는 함수의 역할을 하고 있는것 같은데 함수 명명으로 넣는건 어떤가여?

Copy link
Member Author

Choose a reason for hiding this comment

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

9ab6c73 editorViewRouter로 명명하였습니다!

this.render();
}
render() {
// 자식 클래스에서 Override

Choose a reason for hiding this comment

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

저번 리뷰때 남긴것처럼 자식 클래스에서 해당 메서드를 강제하고 싶으면 throw를 해주는게 좋아보여요

Copy link
Member Author

Choose a reason for hiding this comment

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

33a4387 적용해보았습니다!

Comment on lines 48 to 54
const res = request("", {
method: "POST",
body: JSON.stringify({
title: "제목 없음",
parent: `${this.state.id}`,
}),
});

Choose a reason for hiding this comment

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

요청을 보낼때 메서드나 직렬화를 컴퍼넌트 내에서 하지말고 별도 서비스를 만들어서 사용하는게 좋아보여요

@@ -0,0 +1,20 @@
const API_END_POINT = "https://kdt-frontend.programmers.co.kr/documents";

Choose a reason for hiding this comment

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

전반적으로 폴더 명을 대문자로 지은것 같은데 컴포넌트 명이 아닌 파일들은 소문자로 짓는게 일반적인 규칙으로 적합할거같아요

Comment on lines 25 to 36
for (const key in docu) {
const { id, title, documents } = docu[key];
const parentDocu = new DocumentItem();
parentDocu.setState({ id, title, isFolded: true });
container.appendChild(parentDocu.el);
if (container.getAttribute("class") !== "container") {
parentDocu.el.setAttribute("style", "display: none");
parentDocu.el.setAttribute("class", "child");
}
if (documents.length !== 0) {
renderDocuments(parentDocu.el, documents);
}

Choose a reason for hiding this comment

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

for -in, for of 대신 map, forEach, reduce등의 메서드를 사용해보려고 노력해보세요

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 32
if (container.getAttribute("class") !== "container") {
parentDocu.el.setAttribute("style", "display: none");
parentDocu.el.setAttribute("class", "child");

Choose a reason for hiding this comment

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

반복적으로 DOM API 를 사용한다면 element의 존재 여부도 확인이 되면 좋으니 utils/dom.js 경로에 classList contains를 확인하는 유틸이나 attribute를 지정하는 메서드를 선언해서 사용하시면 훨씬 코드도 간략해지고 반복적으로 쓰기도 좋을겁니다

Comment on lines 18 to 19
this.el.setAttribute("id", "document-app");
this.el.setAttribute("style", "background-color: rgb(251,251,250)");

Choose a reason for hiding this comment

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

스타일 지정은 클래스로 작성하시죠~

Copy link
Member Author

Choose a reason for hiding this comment

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

적용하기 어려운 구조인 것 같아서 css로 접근해보았습니다.. 6676c27 혹시 이런 구조에서 클래스로 스타일 적용을 하려면 어떻게 하는게 좋을까요?

Copy link

@doggopawer doggopawer 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 23 to 26
window.addEventListener("404-not-found", () => {
// TODO: 404 페이지 렌더링
console.log("404 Not Found");
});

Choose a reason for hiding this comment

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

404 렌더링... 결국 하셨군요!!

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
Member Author

Choose a reason for hiding this comment

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

Comment on lines 25 to 36
for (const key in docu) {
const { id, title, documents } = docu[key];
const parentDocu = new DocumentItem();
parentDocu.setState({ id, title, isFolded: true });
container.appendChild(parentDocu.el);
if (container.getAttribute("class") !== "container") {
parentDocu.el.setAttribute("style", "display: none");
parentDocu.el.setAttribute("class", "child");
}
if (documents.length !== 0) {
renderDocuments(parentDocu.el, documents);
}

Choose a reason for hiding this comment

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

저도 노력해야겠습니다.

Comment on lines 39 to 50
const inputCategory = target.getAttribute("class");
if (timer !== null) {
clearTimeout(timer);
}
timer = setTimeout(() => {
const currentWriting = {
[inputCategory]: target.value,
updatedAt: new Date(),
};
setItem(this.state.id, currentWriting);
savePostOnServer(this.state.id, currentWriting);
}, 1000);

Choose a reason for hiding this comment

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

저도 동일한 부분에서 많이 부족한것 같습니다.

Copy link

@MinwooP MinwooP left a comment

Choose a reason for hiding this comment

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

클래스형 컴포넌트로 구현된 코드를 보는 게 생각보다 어려워서 사실 어떻게 리뷰를 남겨야할까 고민이 많이 됐지만, 404 처리, 제목 수정 시 좌측 documentList에 바로 반영되는 기능, 반복되는 함수 유틸화 등 과제에 많은 공을 들이신 게 느껴지는 코드였습니다 ! 앞으로 다른 과제들이 나올 수록 창기님께 더 여쭤볼 게 많을 것 같네요 ㅎㅎㅎ 과제 너무 고생 많으셨고 앞으로도 화이팅합시다 💪🏻🤗

const { status } = res;
if (status === HTTP_STATUS_RESPONSE_OK) {
return res.json();
} else if (status === HTTP_STATUS_RESPONSE_NOT_FOUND) {
Copy link

Choose a reason for hiding this comment

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

status 값에 따라서 분기처리,,, 꼼꼼하시네요 👍🏻

@@ -0,0 +1,7 @@
let timer = null;
Copy link

Choose a reason for hiding this comment

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

저는 디바운스 유틸화 시키니까 동작을 잘 안하던데 ㅠ 창기님 코드 참고해봐야겠습니다 !

Copy link
Member

@1g2g 1g2g left a comment

Choose a reason for hiding this comment

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

창기님 과제 하시느라 고생 많으셨습니다✨
매사 열심히 하시고 또 잘 하셔서 저에게 많은 자극이 되는 것 같아요!
코드 보고 많이 배워갑니다~~👍

Copy link
Member

Choose a reason for hiding this comment

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

클래스형을 이용해 다른 컴포넌트들에서 'Component'를 상속받아 중복된 코드들을 처리할 수 있다는 점이 좋은 것 같습니다. 창기님께서 이번 과제는 함수형으로 구현한다고 하셨는데 어떻게 처리하셨을지 기대가 됩니다😊

Copy link
Member

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.

5 participants