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

[Mission4/최별] - Project_Notion_VanillaJS #34

Open
wants to merge 9 commits into
base: 3/#4_choibyeol_working
Choose a base branch
from

Conversation

choibyeol
Copy link

@choibyeol choibyeol commented Nov 17, 2022

📌 과제 설명

바닐라 JS만을 이용해 노션을 클로닝합니다.
기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.

// utils 폴더에 url.js 파일을 추가하여야 합니다.
export const NOTION_API = "노션 클로닝 API 주소";

동작 모습

notion_video

파일 구조

스크린샷 2022-11-17 오후 10 35 15

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

기본 요구 사항

  • 글 단위를 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의 편집 페이지로 이동하는 링크를 거는 기능을 추가합니다.
  • 그외 개선하거나 구현했으면 좋겠다는 부분이 있으면 적극적으로 구현해봅니다!

개선해야 할 부분

  • 버그 존재
  • root 경로가 아닌 다른 경로로 들어가면 css가 적용되지 않는 현상
  • sidebar header 클릭 시 기본 화면을 띄우지 않음
  • editor의 내용을 수정하고 다른 document를 클릭했을 때 title만 변경되고 content가 변경되지 않는 현상
  • editor 내용이 저장될 때마다 자동으로 sidebar에 렌더링 되도록 해야함
  • 더블 클릭했을 때 editor title이 사라지는 현상
  • css 개선
  • document tree 토글 되게 만들기
  • 모달창 구현
  • 그 외 보너스 요구사항들
  • Rich한 에디터 만들기
  • 편집기 최하단에 하위 document 링크 추가하기
  • 편집기 내에서 다른 document name으로 링크 거는 기능

새롭게 리뷰 받은 부분

  • 삭제 버튼을 눌렀을 때 루트 document로 돌아가지만 새로고침하지 않으면 편집기에 적용되지 않는 현상
  • 추가 삭제 버튼 padding 값 조절
  • 동일한 문서를 재클릭했을 때 문제
  • li 패딩 에러
  • 불필요 로직 및 css 경로 수정 / 지영님

✅ 피드백 반영사항

✅ PR 포인트 & 궁금한 점

  • 어떤 식으로 구조를 구성할지 결정하는 것이 가장 어려운 것 같습니다 😂
  • 컴포넌트를 먼저 나눠놓고 시작을 했지만 의도와는 다르게 명확하게 잘 나눠진 것도 아닌 것 같고 오히려 구현할 때 복잡해진 느낌이 들었습니다. 컴포넌트를 잘 나눌 수 있는 팁이 있을까요?

Copy link
Member

@LucyYoo LucyYoo left a comment

Choose a reason for hiding this comment

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

안녕하세요 별님! 같은 유리팀 유지영입니다👍

직접 실행을 해보니, 삭제 버튼을 눌렀을 때 루트 document로 돌아가지만 새로고침하지 않으면 편집기에 적용이 안되더라구요! 이 부분도 추가되면 더욱 좋을 것 같아요 :)

첫 개인 프로젝트 너무 수고하셨습니다! 끝까지 마무리하셔서 과제 제출하시는 모습이 감동받았습니다><


.add-btn {
position: absolute;
padding: 0.5rem 1rem;
Copy link
Member

Choose a reason for hiding this comment

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

add-btn 과 delete-btn의 padding이 겹쳐있어서 + 버튼을 누르면 삭제버튼이 눌리는 경우가 생기네용!! padding값을 조절해주면 좋을 것 같아요! 😊

Comment on lines +40 to +50
if (isNew) {
const createdPost = await request("/documents", {
method: "POST",
body: JSON.stringify(post),
});
history.replaceState(null, null, `/documents/${createdPost.id}`);
removeItem(postLocalSaveKey);

this.setState({
postId: createdPost.id,
});
Copy link
Member

Choose a reason for hiding this comment

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

새 페이지를 추가하거나 하위문서가 추가되면 바로 postId로 주소를 넘겨서 postId가 new일 때의 로직은 사용되지 않아도 될 것 같아요! 😊

Comment on lines +83 to +88
editor.setState(
this.state.post || {
title: "",
content: "",
}
);
Copy link
Member

Choose a reason for hiding this comment

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

동일한 문서를 재클릭했을 때 this.state.post 값이 undefined여서 기존의 제목과 내용이 뜨지 않는 것 같아요!

if(this.state.post){
editor.setState(this.state.post)
}

이런 식으로 undefined일 땐 아예 setState가 일어나지 않도록 하는 것도 방법일 것 같아요! 👍


$sidebarBody.addEventListener("click", (e) => {
const target = e.target;
const dataId = target.closest("li").dataset.id;
Copy link
Member

Choose a reason for hiding this comment

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

li의 padding 부분이 클릭 됐을 때

image

이런 에러가 뜨더라구요! dataset이 없을 때 에러가 안 뜨도록 하기 위해서

const dataId = target.closest("li")?.dataset.id;

이런 식으로 하는 것도 좋을 것 같아요 😊

Copy link
Author

Choose a reason for hiding this comment

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

이부분도 고민 중이었는데 일단 실행은 되니까 넘어갔었어요 핳ㅎㅎ.. 좋은 방법 감사합니다!!

const { postId } = this.state;

if (postId !== "new") {
const post = await request(`/documents/${[postId]}`);
Copy link
Member

@LucyYoo LucyYoo Nov 18, 2022

Choose a reason for hiding this comment

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

postId가 배열로 들어가는 이유가 따로 있을까용?? 😊

Copy link
Author

Choose a reason for hiding this comment

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

앗 제가 강의 보면서 잘못 친거 같네요...

this.setState = (nextState) => {
this.state = nextState;
$editor.querySelector("[name=title]").value = this.state.title;
$editor.querySelector("[name=content]").innerHTML = this.state.content;
Copy link
Member

Choose a reason for hiding this comment

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

페이지의 내용을 수정하고 다른 list를 누르면 content 내용이 해당 list의 content가 아니라 수정했던 이전 list의 content가 계속 떠 있더라구요! 아마 이 부분이 value가 아니라 innerHTML로 되어있어서 그런 것 같습니다 😊

Copy link
Author

Choose a reason for hiding this comment

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

헉 지영님 고마워요...!!! 역시 똑똑이 지영님🥺

<html>
<head>
<title>Star notion</title>
<link rel="stylesheet" href="src/style/style.css" />
Copy link
Member

Choose a reason for hiding this comment

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

특정 document를 클릭하고 새로고침을 하면 css가 날라가서 찾아보니! css도 스크립트처럼 상대경로가 아닌 절대경로로 지정해주니 해결되는 것 같네요! 😊
<link rel="stylesheet" href="/src/style/style.css" />

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 +25 to +27
// const fetchNewPost = async (postId) => {
// push(`/documents/${postId}`);
// };
Copy link

Choose a reason for hiding this comment

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

주석은 삭제하는 습관을 들이는게 좋겠어요! git이 관리해주니까요:-)

Comment on lines +25 to +29
/* 연속으로 입력을 하고 있을 때는 계속 이벤트 발생을 지연시키다가
입력을 멈췄을 때, 즉, 마지막으로 이벤트가 발생하고 일정 시간이 지났을 때
지연시켰던 이벤트를 실행시키는 것 - 디바운스
디바운스를 이용하면 이벤트 발생하는 횟수를 줄일 수 있다. -> 성능, 최적화
*/
Copy link

Choose a reason for hiding this comment

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

이러한 주석은 readme에 작성하면 좋을 것 같습니다ㅎㅎ

Comment on lines +48 to +51
$addBtn.innerHTML = "+";
const $deleteBtn = document.createElement("button");
$deleteBtn.className = "delete-btn";
$deleteBtn.innerHTML = "x";
Copy link

Choose a reason for hiding this comment

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

이부분도 textContent로 처리할 수 있었을 것 같네요ㅎㅎ

Copy link

@Heojiyeon Heojiyeon left a comment

Choose a reason for hiding this comment

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

별님 안녕하세요!
사이드 바를 header, body, footer로 쪼갠 점이 인상 깊었고, 지영님께서 엄청 꼼꼼히 달아주셔서 저는 살짝 수정하면 좋을 내용 적고 갑니다~
수고 많으셨습니다 :)

Comment on lines +5 to +13
export default function PostEditPage({ $target, initialState }) {
const $postEditPage = document.createElement("div");
$postEditPage.className = "post-edit-page";

this.state = initialState;

let postLocalSaveKey = `temp-post-${this.state.postId}`;

const post = getItem(postLocalSaveKey, {

Choose a reason for hiding this comment

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

전반적으로 new 방어코드 처리를 추가해주시면 좋을 것 같습니다 :)

Comment on lines +27 to +31
/*
sidebar 배경색: rgb(250, 250, 249)
sidebar hover 색: rgb(230, 230, 230)
sidebar 눌러진 페이지: rgb(238, 238, 238)
*/

Choose a reason for hiding this comment

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

이 부분은 제거가 되면 좋을 것 같네요 :)

Copy link

@KSoonYo KSoonYo 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 +31 to +37
this.render = () => {
$target.appendChild($sidebar);
$sidebar.appendChild($sidebarHeader);
$sidebar.appendChild($sidebarBody);
$sidebar.appendChild($sidebarFooter);
};

Copy link

Choose a reason for hiding this comment

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

현재 sidebar를 컨테이너로 하신 것 같은데, 그러면 하위의 sidebarHeader, sidebarBody, sidebarFooter 에 타겟으로 sidebar 컨테이너를 넘겨주기만 해도 이러한 render 함수는 생략할 수 있지 않을까요?

new를 하는 시점에서 sidebar 컨테이너에 append가 되고, 어차피 렌더링은 innerHTML로 처리하니까요! 혹시 이후에 render 함수를 쓰는 일이 있나 봤는데 없는 것 같아서 말씀드려봅니당

Comment on lines +23 to +28
setState: this.setState(),
});

new SidebarFooter({
$target: $sidebarFooter,
setState: this.setState(),
Copy link

Choose a reason for hiding this comment

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

함수를 props로 전달할 때에는 호출하지 않고 넘겨주셔야 더 안전할 것 같아요! this.setState만 넘겨주고 하위 컴포넌트에서 실행이 필요한 경우에만 setState() 로 실행하는 게 맞아 보입니다.

그리고 부모 컴포넌트의 setState를 자식 컴포넌트에서 실행하게 되면 나중에 상태관리가 꼬이게 되니까 다른 방법을 고민해봐야 할 것 같아요. 강의에서처럼 onClick과 같은 트리거 함수를 정의해서 자식에게 전달하고, 그 트리거 함수를 실행하면 부모 컴포넌트에서 setState를 실행하는 게 더 좋지 않을까 의견드려봅니다!

Choose a reason for hiding this comment

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

제 생각에도 append 목적으로는 각 컴포넌트에서 해주는게 좋을것 같고
상위 setState를 통해 하위 state를 관리하는 방향이 더 좋을것 같습니다~
만약 상위의 상태값을 하위에서 바꿔야하는 로직(이벤트)이 있다면 해당 setState를 가져오는게 아닌 해당 로직을 호출하면서 내부에서 변경하는게 좋을것 같습니다~

//ex
new SidebarFooter ({
onClick : () => {
const nextState = 변경로직
this.setState(nextStatee)
}
})

Copy link

@IGhost-P IGhost-P left a comment

Choose a reason for hiding this comment

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

안녕하세요~ 같은팀 양상우 입니다!
UI적으로 컴포넌트를 분리하고 분리함에 따라 생기는 state관리에 신경을 쓰신게 보입니다!!! 잘 봤습니다!

질문에 대한 답변은 팀원들 전반적으로 고민이 있는 부분 같습니다!
다들 한번 얘기 나눠보는것도 좋을것 같아요~!~!

Comment on lines +6 to +11
const $sidebar = document.createElement("div");
const $postEditPage = document.createElement("div");

$target.appendChild($sidebar);
$target.appendChild($postEditPage);

Choose a reason for hiding this comment

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

중복 로직은 묶에서 해결하는 것도 좋을것 같아요~!


this.render();

$editor.addEventListener("keyup", (e) => {

Choose a reason for hiding this comment

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

e를 받아올때 한번에 {target}으로 분리해주는것도 좋을것 같아요~

Comment on lines +23 to +28
setState: this.setState(),
});

new SidebarFooter({
$target: $sidebarFooter,
setState: this.setState(),

Choose a reason for hiding this comment

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

제 생각에도 append 목적으로는 각 컴포넌트에서 해주는게 좋을것 같고
상위 setState를 통해 하위 state를 관리하는 방향이 더 좋을것 같습니다~
만약 상위의 상태값을 하위에서 바꿔야하는 로직(이벤트)이 있다면 해당 setState를 가져오는게 아닌 해당 로직을 호출하면서 내부에서 변경하는게 좋을것 같습니다~

//ex
new SidebarFooter ({
onClick : () => {
const nextState = 변경로직
this.setState(nextStatee)
}
})

Comment on lines +14 to +15
$renderList.innerHTML = "";
this.render();

Choose a reason for hiding this comment

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

이 부분 로직은 계속 중복 되는것 같습니다!! State 랜더를 담당하는 부분까지 역할이 과중 된것 같습니다

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