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

Open
wants to merge 16 commits into
base: 4/#5_eugene028
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added asset/notion.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 18 additions & 0 deletions index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<!DOCTYPE html>
<html lang="ko">

Choose a reason for hiding this comment

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

🇰🇷

<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<link rel="icon" href="asset/notion.png"/>

Choose a reason for hiding this comment

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

오...이런 디테일 완전 멋있습니다..! 👋

Copy link
Author

Choose a reason for hiding this comment

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

ㅎㅎ 감사합니다!

<link rel="stylesheet" href="/style/global.css">
<link rel="stylesheet" href="/style/DocumentList.css">
<link rel="stylesheet" href="/style/DocumentModal.css">
<link rel="stylesheet" href="/style/DocumentEdit.css">
<link rel="stylesheet" href="/style/DocumentDelete.css">
<title>유진의 Notion</title>
</head>
<body>
<main class ="app"></main>
<script src="/src/main.js" type ="module"></script>
</body>
</html>

Choose a reason for hiding this comment

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

😢

Copy link
Author

Choose a reason for hiding this comment

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

html파일까지 미처 신경 못썼네요!! 수정하도록 하겠습니다 🫡

30 changes: 30 additions & 0 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { EditorPage } from "./EditorPage.js";

Choose a reason for hiding this comment

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

코드 너무 깔끔하고 보기 편해요...! 깔끔하게 짜시는 게 습관이 되신 것 같아요 👍

Copy link
Author

Choose a reason for hiding this comment

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

헉 감사합니다 !! 🥹

import { DocumentPage } from "./DocumentPage.js";
import { initRouter } from "./router.js";

export default function App ($target) {
const $app = document.createElement('div');
$app.className = 'mainApp'

Choose a reason for hiding this comment

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

다음부터는 표준도 참고해보세요 ㅎㅎ
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList

Copy link
Author

Choose a reason for hiding this comment

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

classList가 표준이군요! DOM의 클래스를 관리할 때 클래스 이름이 하나만 존재한다면 className으로 관리하는 것이 편리할 수 있으나, 여러개의 클래스를 가지고 있을때 이전에 어떤 class가 존재하든지 상관없이 교체해버리거나 중복된 class명을 덮어씌울 수 있다는 점도 있네요!! 다양한 메서드를 이용할 수 있는 classList를 이용해 보도록 해야겠습니다!! 메서드를 이용할 때 근거를 가지고 사용하는 것이 중요한 것 같은데 멘토님 덕분에 찾아보며 배우게 되는 것 같습니다 👍

const editorPage = new EditorPage($app);
const documentPage = new DocumentPage($app);

$target.appendChild($app);

this.route = () => {

Choose a reason for hiding this comment

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

EditorPage 를 먼저 선언해놓고 route 할 때마다 setState 를 바꿔주는 방식으로 구현하셨군요...! 이 방법으로 하니까 바로바로 읽히네요...!!

Copy link
Author

Choose a reason for hiding this comment

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

ㅎㅎ 맞습니다! EditorPage에서 url을 가져와 그때그때 문서의 id를 수정해 주는 방식으로 state를 만들어보려고 하였습니다!

const { pathname } = window.location;
if (pathname.indexOf('/documents/') === 0){

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

if(pathname.includes('/documents/'))

이렇게 작성하면 더욱 간결하게 코드를 작성할 수 있을 것 같군요!! 이 메서드도 적극적으로 이용해도보록 하겠습니다 감사합니다!

const [, , documentId] = pathname.split('/');
editorPage.setState({...editorPage.state, id : documentId});
}
}
this.render = () => {
documentPage.render();
const { pathname } = window.location;
const [, , documentId] = pathname.split('/');
if(documentId){
editorPage.setState({...editorPage.state, id: documentId})
}
}
this.render()
initRouter({onRoute : () => this.route()});

Choose a reason for hiding this comment

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

Suggested change
initRouter({onRoute : () => this.route()});
initRouter({onRoute : this.route});

이렇게는 안될까요?

Copy link
Author

Choose a reason for hiding this comment

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

지금 코드를 다시 보니까 콜백 형태로 넘겨줄 필요는 없는 것 같네요..ㅎㅎ

onClick = { () => func() }
onClick = {func}

옛날에 리액트를 공부하다가 이런 질문을 본 적이 있는데 항상 어떤 차이인지 궁금했었습니다!
제가 이해하기로는, onClick 의 props로 func 함수를 넘겨줄 때, 첫번째 경우는 바로 func()을 실행해야 하는 경우인 것이고, 두번째 경우는 func를 단순이 인자로 넘겨지는 것으로, onClick에서 추후 제가 현재 작성한 initRouter 처럼 함수 내부에서 알아서 다시 호출해주는 것의 차이라고 이해하였는데, 제가 이해한 부분이 맞는 것인지 궁금합니다!

Choose a reason for hiding this comment

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

ㅎㅎ 이것은 리액트와 조금 다른 결로 생각하시는게 좋아요
다만 말씀하신게 다 맞습니다!
공부 잘하셨네용

}
34 changes: 34 additions & 0 deletions src/Components/ChildDocuments.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { push } from '../router.js';

export default function ChildDocument ({$target, $document}){
const $childDocument = document.createElement('div');
$childDocument.className = 'childLink';
$target.append($childDocument);

this.state = {
data : $document
}
this.setState = nextState => {
this.state = nextState
this.render();
}
this.render = () => {
if(this.state.data){
const { documents } = this.state.data
if(documents){
$childDocument.innerHTML = `${
documents.map(({id, title}) =>
`<li class="linkDoc" data-id=${id}>${title}</li>`
).join('')}`
}
}
}

this.render();

$childDocument.addEventListener('click', (e) => {
const $link = e.target.closest('li');
const { id } = $link.dataset;
push(`/documents/${id}`)
})
}
33 changes: 33 additions & 0 deletions src/Components/DocumentCreate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { DocumentModal } from "./DocumentModal.js";

export function DocumentCreate({$target, parentId, onSubmit}){
this.state = {
parentId :null,
}
this.setState = (nextState) => {
this.state = nextState;
}
this.render = () => {
const $createBtn = document.createElement('button');
$createBtn.className = 'createDoc';
if (parentId === null){
$createBtn.className = 'rootCreate'
}
$createBtn.textContent = '+';
$target.append($createBtn);
}
$target.addEventListener('click', (e) => {
const $createBtn = e.target.closest('button');
if($createBtn){
if($createBtn.classList.contains('rootCreate')){
e.stopImmediatePropagation();
const modal = new DocumentModal(null , onSubmit)
modal.modalOpen();
return
}
const { id } = $createBtn.nextElementSibling.dataset
const modal = new DocumentModal(id , onSubmit)
modal.modalOpen();
}
})
}
22 changes: 22 additions & 0 deletions src/Components/DocumentDelete.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { deleteDocuments } from "../api.js";
import { push } from "../router.js";

export default function DocumentDelete ({$target, id}){
const $deleteBtn = document.createElement('button');
$deleteBtn.className= 'delete-btn'
$target.appendChild($deleteBtn)
$deleteBtn.textContent = '삭제하기'

this.state = {
id : id,
}

Choose a reason for hiding this comment

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

한줄로 줄일 수 있을것 같습니다!
this.state = { id }

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
}
$deleteBtn.addEventListener('click', async (e) => {
await deleteDocuments(this.state.id)
alert("삭제 완료");
push('/');
location.reload();

Choose a reason for hiding this comment

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

오 reload 메소드를 통해서 바로 reload하는게 유저 입장에서 좋은것 같습니다! 하나 배워갑니다! 👍

Copy link
Author

Choose a reason for hiding this comment

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

사실 state가 변경되는 것을 바로바로 볼 수 있도록 해주고 싶은데, 그 방법이 생각이 잘 나지 않아서 강제로 reload를 하게 되었어요
새로고침을 하게 되면 UI가 깜빡이기 때문에 유저 입장에서는 좋은 방법은 아닌 것 같아서 개인적으로는 살짝 구현에 있어 아쉬운 부분이라고 생각됩니다! 😂

})
}
95 changes: 95 additions & 0 deletions src/Components/DocumentList.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { DocumentCreate } from "./DocumentCreate.js"
import { push } from '../router.js';

export function DocumentList({$target, data =[], initialState, onSubmit}) {
this.state = initialState
this.setState = (nextState) => {
this.state = nextState
}
this.render = ($renderDOM = $target) => {
data.map((data) => {
const doc = document.createElement('li');
doc.setAttribute("data-id", `${data.id}`);
doc.setAttribute("class", 'doc');
doc.textContent =`${data.title}`
$renderDOM.insertAdjacentElement("beforeend", doc);
})

Choose a reason for hiding this comment

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

아주 좋지 못한 코드입니다 ㅠㅠ 왜그럴까요?

Copy link
Author

Choose a reason for hiding this comment

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

data를 루프할때마다 DOM요소를 새롭게 생성하기 때문에 성능 면에서 굉장히 좋지 못한 것 같습니다. DOM 요소에 접근하기 위해서는 많은 리소스가 필요한데, 대용량 data를 넘겨받을 경우에 정말 느린 속도로 동작할 것 같습니다. 그리고 마지막에 insertAdjacentElement를 이용하여 기존의 DOM에 내용들을 추가해 넣는 방식이 render의 동작을 느리게 할것으로 보입니다.

모든 데이터를 한번에 랜더링하지 않고, 부모 노드를 클릭하였을 때에만 그에 대한 하위 노드들을 랜더링하고 싶어서 반복문을 이용하여 데이터에� 대한 DOM 요소를 만들고자 이러한 코드를 작성하게 된 것 같습니다 🥲

Choose a reason for hiding this comment

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

굿굿
아주아주아주 잘 아시네요
꼭 수정해보세요!

}

this.onSelect = (data, $li) => {
if(data.length === 0){
const $haveNothing = document.createElement('div');
$haveNothing.classList.add('nothing')
$haveNothing.textContent = '하위 페이지 없음'
$li.append($haveNothing)
}

Choose a reason for hiding this comment

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

자식이 없다면 바로 return으로 끝내주는게 좋아보입니다!

Copy link
Author

Choose a reason for hiding this comment

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

넵 !! 확인하고 수정하겠습니다 :)

data.forEach((data => {
const $parentNode = document.createElement('div')
$parentNode.className = `doc-${this.state.selectedNode}`
$parentNode.style.marginLeft = `${this.state.depth * 10}px`;
const createBtn = new DocumentCreate({
$target: $parentNode,
parentId: this.state.parent,
onSubmit: onSubmit
})
createBtn.render();
$li.append($parentNode);
const documentList = new DocumentList({
$target: $parentNode,
data: [data],
depth: this.state.depth + 1,

Choose a reason for hiding this comment

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

depth 도 initialState 안에 포함돼 있는 프로퍼티 아닌가요...! 호출 시 5개의 파라미터를 전달해주게 됩니다..!

Copy link
Author

Choose a reason for hiding this comment

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

헉!! 그러네요!!! 이부분을 꼼꼼하게 확인못했습니다!! 수정해두겠습니다~~

initialState: this.state,
onSubmit: onSubmit
})
documentList.render();
}))
}
$target.addEventListener('click', (e) => {
e.stopPropagation();
if($target.classList.contains('documentPage') && !e.target.classList.contains('doc'))
return
else if (e.target.classList.contains('createDoc')){
return
}
else if (e.target.classList.contains('nothing')){
return
}
Comment on lines +49 to +56

Choose a reason for hiding this comment

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

중괄호가 오락가락하고 있어서 위험할 수 있습니다!

Copy link
Author

Choose a reason for hiding this comment

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

일관성을 지켜서 쓸거면 쓰고, 안쓸거면 아예 쓰지를 말아야겠군요..!!!

if(this.state.isOpen){
while(e.target.querySelector('div')){
e.target.classList.remove("open");
const $removeTarget = e.target.querySelector('div')
e.target.removeChild($removeTarget);
}
this.setState({
...this.state,
isOpen: !this.state.isOpen,
})
return
}
Comment on lines +59 to +70

Choose a reason for hiding this comment

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

의도와는 조금 다른 방식으로 동작하는 것 같습니다. 루트 컴포넌트에서 하위 컴포넌트를 보여줄 때에는 정상적으로 동작하지만 하위 컴포넌트의 하위 컴포넌트를 보여줄 때에는 이미 isOpen이 true인 상태여서 두 번 클릭해야 합니다. isOpen을 각각 적용 시키는 방법은 어떨지 생각해봤습니다!

Copy link
Author

Choose a reason for hiding this comment

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

각각의 컴포넌트별로 isOpen의 state를 관리할 수 있도록 리팩토링해보도록 하겠습니다! 감사합니다 🫡

const $li = e.target
if($li){
const { id } = $li.dataset;
if(data){
const childrenData = data.map(data => data.documents)
$li.classList.add('open')
if(childrenData[0].length > 0){
this.setState({
parent: this.state.depth === 0 ? parseInt(id) : parseInt(this.state.parent),

Choose a reason for hiding this comment

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

parseInt() 사용시 radix는 꼭 넣어주셔야합니다

Copy link
Author

Choose a reason for hiding this comment

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

지금까지 습관적으로 parseInt를 사용시에 radix를 넣어주지 않고 있었네요!! (당연히 10진수라고 생각해서..)
앞으로 radix를 넣는 것을 습관화하도록 하겠습니다!

selectedNode: parseInt(id),
isOpen: !this.state.isOpen,
depth: this.state.depth + 1
})
this.onSelect(childrenData[0], $li)
}

Choose a reason for hiding this comment

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

하위 컴포넌트가 있는 하위 컴포넌트를 열고 닫을 때 마다 depth가 증가합니다! 위에서 depth에 따라 margin을 다르게 주었기 때문에 계속 옆으로 밀려나는 듯 합니다!

Copy link
Author

Choose a reason for hiding this comment

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

헉!! 그런 에러가 존재하는군요, 열고 닫을 때마다 isOpen 상태가 true일때는 depth가 늘어나지 않도록 수정해야겠습니다. 꼼꼼한 리뷰 감사드립니다!

else {
this.setState({
...this.state,
isOpen: !this.state.isOpen,
})
this.onSelect([], $li)
}
push(`/documents/${id}`);
}
}
})
}
42 changes: 42 additions & 0 deletions src/Components/DocumentModal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
export function DocumentModal(id , onSubmit){
const $app = document.querySelector('.app');
const $modalContainer = document.createElement('div')
$modalContainer.className = 'modal';
$app.appendChild($modalContainer);
this.render = () => {
$modalContainer.innerHTML = `
<div class="modal-content">
<form>
<input class ="modalText" type="text" placeholder='문서의 제목을 작성해주세요'/>
</form>
<button class="closeBtn" id="close-modal">❌</button>
</div>
`
}

$modalContainer.addEventListener('submit', async (e) => {
e.preventDefault();
const $input = $modalContainer.querySelector('.modalText');
const content = $input.value;
await onSubmit(content, id);
$input.value =''
alert('문서 생성이 완료되었습니다')
const modal = document.querySelector('.modal');
modal.remove()
})

$modalContainer.addEventListener('click', (e) => {
const $closeBtn = e.target.closest('button')
if ($closeBtn.classList.contains('closeBtn')){
const modal = document.querySelector('.modal');
modal.remove()
}
Comment on lines +34 to +37

Choose a reason for hiding this comment

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

하위 페이지를 생성할 때 입력하는 부분에 클릭을 하게 되었는데 오류가 발생했습니다. $closeBtn의 존재에 대한 조건도 추가하면 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

현재 클릭 이벤트가 $closeBtn 한정으로 일어나지 않는 것으로 보이네요.
이에 대해서 가드할 수 있는 코드를 추가해 넣도록 하겠습니다. 감사합니다!

})

this.modalOpen = () => {
const modal = document.querySelector('.modal');
modal.style.display = "block";
document.body.style.overflow = "hidden";
}
this.render()
}
40 changes: 40 additions & 0 deletions src/Components/Editor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
export default function Editor({$target, initialState = {
title: '',
content: '',
}, onEditing}){
const $editor = document.createElement('div');
$editor.className = 'editor'
$target.appendChild($editor);
this.state = initialState;
let isinitialize = false

Choose a reason for hiding this comment

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

요건 왜 상태가 아닐까요?

Copy link
Author

Choose a reason for hiding this comment

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

isinitialize는 한번 유효성을 체크하고 말 변수라고 생각하여 상태에 넣지 않았습니다!
상태가 계속하여 변화하는 것이나, 외부로부터 받아오게 되는 변수들을 state에 넣어서 관리해야 한다는 생각을 가지고 있었는데, 사실 로컬에서 관리하는 state이기 때문에 해당 변수도 상태관리해도 좋을 것 같다는 생각이 듭니다.!!

Choose a reason for hiding this comment

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

사실 isinitialize라는 상태 자체가 존재하면 안된다고 생각합니다 ㅎㅎ


this.setState = (nextState) => {
this.state = nextState
$editor.querySelector('[name=content]').value = this.state.content;
$editor.querySelector('[name=title]').value = this.state.title;
this.render();
}
this.render = () => {
if(!isinitialize){
$editor.innerHTML = `
<input type="text" class="titleInput" name="title"/>
<textarea name="content" class="contentInput"/>
`
isinitialize = true
}
}

this.render();
$editor.addEventListener('keyup', e => {
const { target } = e
const name = target.getAttribute('name')
if(this.state[name] !== undefined) {
const nextState = {
...this.state,
[name]: target.value
}
this.setState(nextState)
onEditing(this.state)
}
})
}
Loading