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 #24

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

Conversation

solar3070
Copy link
Member

@solar3070 solar3070 commented Nov 16, 2022

📌 과제 설명

바닐라 JS만을 이용해 노션을 클로닝합니다.
현재 미완성된 기능이 많아서 코드리뷰 기간에도 계속해서 개발하고 스타일링하려고 합니다.

배포 링크

https://juns-notion.netlify.app/
페이지 생성, 수정, 삭제시 열려있던 하위 페이지들이 모두 닫히는 현상이 있습니다.. 😂

image

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

기본 요구사항

기본적인 레이아웃은 노션과 같으며, 스타일링, 컬러값 등은 원하는대로 커스텀합니다.
글 단위를 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의 편집 페이지로 이동하는 링크를 거는 기능을 추가합니다. (구현 예정)

✅ PR 포인트 & 궁금한 점

  1. 환경변수를 사용하고 싶었는데 process is not defined라는 에러가 계속 발생하였습니다. 바닐라 자바스크립트로 환경변수를 설정해서 사용하는 것이 가능한가요?
  2. 현재 포스트 리스트(사이드바)가 생성, 수정, 삭제 시에 계속 리렌더링이 되다 보니 열려있던 하위 페이지들이 모두 닫히고 있는데 이를 해결하기 위해 좋은 방법이 뭐가 있을까요?
  3. 이벤트 관련 코드를 App.js의 하단에 작성하였는데 이 위치말고 더 적절한 부분이 있을까요?
  4. PostList와 SubPostList 컴포넌트가 코드가 많이 비슷해서 renderPosts 함수를 따로 뺐는데 뭔가 더 중복 코드를 줄일 방법이 있을까요?
  5. 어떤 부분이든 개선할 점이 있을까요? (불필요한 코드, 렌더링, 데이터 흐름, 의존성 등)

Copy link

@tooooo1 tooooo1 left a comment

Choose a reason for hiding this comment

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

혜준님 같이 팀해서 너무 좋았어요!
마지막 리뷰하러 방문했어용. 프로젝트 하느라 고생 많이하셨습니다.

@@ -0,0 +1,12 @@
<!DOCTYPE html>
<html lang="en">
Copy link

Choose a reason for hiding this comment

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

ko면 더 좋을 거 같아요

Comment on lines +32 to +34
if (className === "no-sub-page") {
return;
}
Copy link

Choose a reason for hiding this comment

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

좋네요

Copy link

@chunwookJoo chunwookJoo left a comment

Choose a reason for hiding this comment

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

혜준님 노션 프로젝트 하시느라 고생하셨습니다!
꼼꼼한 성격답게 더미데이터로 체크하면서 코딩하신것 같아서 많이 배워가는 것 같습니다.
페이지 생성이나 삭제할때 토글리스트 닫히는건 혜준님이면 금방 하실것 같아요~~
한달동안 수고하셨고 다른 팀 가셔도 화이팅 하세용~

if (!isInitialize) {
$editor.innerHTML = `
<input type="text" name="title" style="width: 600px;" value="${this.state.title}" />
<textarea name="content" style="width: 600px; height: 400px;">${this.state.content}</textarea>

Choose a reason for hiding this comment

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

style.scss파일이 있는데 여기서 따로 지정해둔 이유가 있을까요??
그리고 혹시 textarea 사이즈 고정을 원하시면 resize:none 을 주셔도 좋을 것 같습니당

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 스타일링 코드는 다른 커밋에서 scss파일로 옮겨두었습니다!
사용자가 마음대로 늘리게 하고 싶어서 resizeL none은 주지 않았어요 :)

if ($li) {
const { id } = $li.dataset;
const { className } = e.target;
if (className === "post-delete") {
onDelete(id);

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.

생각해보니 지금 사용자가 실수로 사용했을 경우를 방지하지 않고 있어서 충분히 당황할 수 있을 것 같네요.
모달 창으로 삭제 여부를 물어보거나 노션처럼 삭제 후 되돌리기 기능을 넣는 것도 좋을 것 같아요! 의견 감사합니다 👍

@@ -40,6 +40,11 @@ export default function PostEditPage({ $target, initialState }) {
}

removeItem(postLocalSaveKey);

this.setState({
postId: post.id + "",

Choose a reason for hiding this comment

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

저는 이거 일일히 toString()으로 사용했는데 이렇게 하면 훨씬 간결해지네요 👍

await request(`/documents/${postId}`, {
method: "DELETE",
});
};

Choose a reason for hiding this comment

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

이렇게 한 곳에 모아두니까 너무 보기 좋네요 배워갑니다!

font-size: 20px;
cursor: pointer;

&:hover {

Choose a reason for hiding this comment

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

아 &를 이럴때 쓰는거네요..
저는 hover 효과 줄 때
.toggle-button:hover 이런식으로 하나를 또 만들었는데ㅋㅋㅋ 배워갑니당

}
}

.no-sub-page {
padding-left: 39px;
user-select: none;

Choose a reason for hiding this comment

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

이 user-select:none은 무슨 역할을 하나요??

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

@beni1026 beni1026 left a comment

Choose a reason for hiding this comment

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

안녕하세요 혜준님 코드리뷰 하러 왔습니다!

과제하시느라 수고 많으셨습니다 ~~
페이지 배포까지 하시다니 대단하세요!!
기능도 잘 작동하고 css 공들이신게 느껴질 정도로 노션과 똑같이 구현하셨네요 ~~ 👍
다음 과제도 화이팅 입니다!

Comment on lines +1 to +10
/* 컬러 코드*/
$BROWN_10: #fbfbfa;
$BROWN_20: #ebebea;
$BROWN_30: #dddddc;
$BROWN_40: #d3d1cb;
$BROWN_50: #b5b5b2;
$BROWN_60: #908f8c;
$BROWN_70: #676662;
$BROWN_80: #5e5c57;
$BROWN_90: #37352f;

Choose a reason for hiding this comment

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

오 컬러코드 미리 지정해서 사용하는 거 좋네요

Comment on lines +1 to +43
export const renderPosts = (root, posts, sub, down = false) => {
let $ul = document.createElement("ul");
if (down) {
$ul.classList.add("post-hide", "sub-post");
}
root.appendChild($ul);

posts.forEach((post) => {
let $li = document.createElement("li");
$li.setAttribute("data-id", post.id);
$li.innerHTML = `
<div class="post">
<div>
<span>
<button class="toggle-button" data-is-opened="false">&#9656;</button>
</span>
<span class="post-title">${
post.title ? post.title : "제목 없음"
}</span>
</div>
${
!sub
? `
<div class="button-group">
<button class="post-create">&#10010;</button>
<button class="post-delete">&#10006;</button>
</div>`
: ""
}
</div>
`;

$ul.appendChild($li);
if (post.documents.length) {
renderPosts($li, post.documents, sub, true);
} else {
let $ul = document.createElement("ul");
$ul.setAttribute("class", "post-hide");
$ul.innerHTML = `<span class="no-sub-page">하위 페이지 없음</span>`;
$li.appendChild($ul);
}
});
};

Choose a reason for hiding this comment

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

코드 보니까 서브 페이지 링크 구현 방식에 대해 이해가 되네요 👍 저도 나중에 구현해보겠습니다

Comment on lines +1 to +17
export const toggle = ($button, $li) => {
const { dataset, classList } = $button;
const isOpened = JSON.parse(dataset.isOpened);
const $ul = $li.querySelector("ul");
if ($ul) {
!isOpened ? $ul.classList.remove("post-hide") : $ul.classList.add("post-hide");
}
dataset.isOpened = !isOpened;
if (!isOpened) {
classList.add("open-toggle");
} else {
classList.add("close-toggle");
setTimeout(() => {
classList.remove("open-toggle", "close-toggle");
}, 300);
}
};

Choose a reason for hiding this comment

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

토글 방식을 class를 이용해서 하는 방법이 있군요 배워갑니다 !!

Copy link

@fly1chop fly1chop left a comment

Choose a reason for hiding this comment

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

안녕하세요 혜준님~ 프로젝트 하느라 수고 많으셨습니다~
배포까지 하셔서 직접 사용해볼수있어서 너무 좋았습니다 ^^ 스타일링도 아주 깔끔하게 하셨구요..!!
editing하는 부분에 의견 살짝 남겨봤습니다~

Comment on lines +37 to +40
let $ul = document.createElement("ul");
$ul.setAttribute("class", "post-hide");
$ul.innerHTML = `<span class="no-sub-page">하위 페이지 없음</span>`;
$li.appendChild($ul);

Choose a reason for hiding this comment

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

이거 좋네요!

Comment on lines +38 to +39
window.dispatchEvent(new CustomEvent("update-tree"));
}

Choose a reason for hiding this comment

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

저도 이 부분 어떻게 처리할지 고민 많이 했는데 커스텀 이벤트를 활용하는것이 좋네요..!! 이것도 나중에 변수로 만들어내면 더 깔끔할것같다는 생각입니다~

Comment on lines +43 to +46
this.setState({
postId: post.id + "",
post,
});

Choose a reason for hiding this comment

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

editing 할때마다 this.setState => editor.setState하게되어 저장하면 form focus가 없어집니다. 제 생각에는 editing할때는 setState 없이 api 업데이트만 하고 page reload, push state할때만 setState 하면 더 좋을것같습니다~

Copy link
Member Author

Choose a reason for hiding this comment

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

오 안그래도 이 부분 고민했었는데 감사합니다! 더 고민해보겠습니다

@sairion
Copy link

sairion commented Nov 23, 2022

환경변수를 사용하고 싶었는데 process is not defined라는 에러가 계속 발생하였습니다. 바닐라 자바스크립트로 환경변수를 설정해서 사용하는 것이 가능한가요?

process.env는 Node 환경에 존재하는 환경변수입니다. (process라는 전역변수가 존재) node 환경의 전역변수나 라이브러리들을 변환해주는 browserify라는 프로젝트가 있었는데 이것이 Webpack으로 발전했다고 보시면 됩니다. 그래서 결론적으론, 이런 컴파일 과정을 거쳐야만 사용할 수 있습니다. 웹에서만 이용 가능한 환경변수를 사용하고 싶다면, 그냥 전역변수를 사용하시면 됩니다. ㅎㅎ;; process.env나 window나 프로그래밍 관점에선 모두 전역 오브젝트이기에, 사실 다른 점은 없다고 보셔도 됩니다.

현재 포스트 리스트(사이드바)가 생성, 수정, 삭제 시에 계속 리렌더링이 되다 보니 열려있던 하위 페이지들이 모두 닫히고 있는데 이를 해결하기 위해 좋은 방법이 뭐가 있을까요?

근본적으론 포스트들의 오브젝트 모델을 가지고 DOM과 싱크하는 형태로 구조를 바꾸고, 필요한 부분만 렌더링되게 하는 방식으로 구현되어야 전체가 다 렌더링되는 문제가 근본적으로 해결될 수 있을 것 같아요. 혹은, rerender를 할 때 열고 닫혀있는 페이지들의 리스트를 user state를 저장하는 오브젝트 따위를 따로 두고 렌더링되는 시점에 이 state가 반영되게 하는 식으로도 문제 우회는 가능할 것 같아요.

이벤트 관련 코드를 App.js의 하단에 작성하였는데 이 위치말고 더 적절한 부분이 있을까요?

앱이 시작하는 시점에서 초기화하는 것은 괜찮은 것 같습니다!

PostList와 SubPostList 컴포넌트가 코드가 많이 비슷해서 renderPosts 함수를 따로 뺐는데 뭔가 더 중복 코드를 줄일 방법이 있을까요?

공통화를 잘 하신 것 같습니다! :)

어떤 부분이든 개선할 점이 있을까요? (불필요한 코드, 렌더링, 데이터 흐름, 의존성 등)
코드 리뷰에 달겠습니다!

},
});

this.route = () => {
Copy link

Choose a reason for hiding this comment

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

route 보단 좀 더 구체적으로 어떤 동작을 하는지 설명하는 onRouteChange 같은 이름이 좋을 것 같아요!


this.render = () => {
$postList.innerHTML = "";
this.state.length && renderPosts($postList, this.state, false);
Copy link

Choose a reason for hiding this comment

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

이런 경우엔 short-circuit보단 (&&) if문을 이용하는 것이 더 보기 좋습니다.

await request(`/documents/${post.id}`, {
method: "PUT",
body: JSON.stringify(post),
});
Copy link

Choose a reason for hiding this comment

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

사용부에서 await을 어짜피 하기 때문에 await을 하기보단 그냥 return을 하는게 낫지 않을까 생각됩니다!

let isInitialize = false;

this.render = () => {
if (!isInitialize) {
Copy link

Choose a reason for hiding this comment

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

isInitialize 라는 별도의 변수를 두기보단, innerHTML이 비어있는지 여부로 체크도 가능할 것 같아요.
(가능하면 변수의 사용을 줄이는 것이 읽기 좋기에..)

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