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

Open
wants to merge 26 commits into
base: 4/#5_colorkite10_working
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
194e851
Chore: 초기 설정
colorkite10 Jul 4, 2023
4255ac7
Feat: api 연결 후 사이드바에 documents들 표시하기
colorkite10 Jul 4, 2023
d709e0e
Feat: +버튼 누를 시 새로운 document 생성
colorkite10 Jul 4, 2023
2bc808d
Feat: Edit page 생성
colorkite10 Jul 4, 2023
2f75227
선택한 document의 content 불러오기 기능 시도 중
colorkite10 Jul 4, 2023
1c54656
Feat: 제목 없고 내용 없는 문서 편집 시 기본 placeholder 문구 설정
colorkite10 Jul 5, 2023
ba96536
Feat: localStorage 세팅 후 document 클릭 시 edit page에 title과 content 띄움
colorkite10 Jul 5, 2023
b2d4e9d
Fix: 새로운 document 생성 시 content가 null이어서 나는 오류 해결
colorkite10 Jul 5, 2023
4be8e7f
Feat: 새로운 root문서 추가 기능 구현
colorkite10 Jul 5, 2023
912686c
Feat: 새로운 root 문서 클릭 버튼 DocumentList에 붙이고 LinkButton 컴포넌트 삭제
colorkite10 Jul 5, 2023
1f23e72
Fix: 보고 있지 않은 문서 삭제 시에도 기본 화면으로 돌아가는 문제 해결
colorkite10 Jul 5, 2023
c9c8484
Feat: documentList 토글 기능 구현
colorkite10 Jul 5, 2023
1223509
Fix: add a document 버튼 누를 시 리스트에 즉시 반영되지 않는 문제 해결
colorkite10 Jul 5, 2023
d25017c
Feat: 상위 document 삭제 시 하위 document도 삭제와 경로 설정
colorkite10 Jul 5, 2023
58316f9
Fix: 하위 문서가 displyed일 때 상위 문서의 이름 클릭 시 토글 닫히는 문제 해결
colorkite10 Jul 5, 2023
2adfa7b
Docs: 주석 제거 및 필요 없는 로직 삭제
colorkite10 Jul 5, 2023
b3776ca
Rename: 파일 구조 변경
colorkite10 Jul 5, 2023
9b40e5c
Design: css 스타일 적용
colorkite10 Jul 5, 2023
495a583
Fix: root document 생성 버튼 아래를 눌러도 새 문서가 생기는 문제 해결
colorkite10 Jul 6, 2023
5f3c043
Fix: root 문서 생성 후 아무런 텍스트 입력하지 않고 다시 root 문서 생성 시 해당 화면에 머무르는 문제 해결
colorkite10 Jul 6, 2023
98a5d09
Fix: 문서 리스트 클릭 시 해당 글 제목과 내용이 안 불러오지던 문제 해결
colorkite10 Jul 6, 2023
95c463a
Docs: console.log 삭제
colorkite10 Jul 6, 2023
76afe40
Try: 무언갈 시도했으나 되지 않은 마지막 지점
colorkite10 Jul 6, 2023
46ff36b
Fix: 못 고치던 오류 타협
colorkite10 Jul 6, 2023
58df77b
Refactor: 불필요한 공백 제거
colorkite10 Jul 18, 2023
fcdca72
Refactor: 불필요한 async 키워드 제거
colorkite10 Jul 18, 2023
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
13 changes: 13 additions & 0 deletions index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!DOCTYPE html>
<html>
<head lang="ko">
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, height=device-height" />
<title>채연스 Notion cloning</title>
</head>
<body>
<main id="notion-app">
<script src="/src/main.js" type="module"></script>
</main>
</body>
</html>
23 changes: 23 additions & 0 deletions src/api/api.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { push } from "../routes/router.js";

const API_END_POINT = "https://kdt-frontend.programmers.co.kr";

export const request = async (url, options = {}) => {
try {
const res = await fetch(`${API_END_POINT}${url}`, {
...options,
headers: {
"Content-Type": "application/json",
"x-username": "colorkite10",
},
Comment on lines +8 to +12

Choose a reason for hiding this comment

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

P1;

이렇게 하게 된다면 headers를 변경하고 싶어도 변경하지 못할 것 같아요.
아래와 같이 headers 또한 수정 가능하게 해두면 어떨까요?

Suggested change
...options,
headers: {
"Content-Type": "application/json",
"x-username": "colorkite10",
},
...options,
headers: {
"Content-Type": "application/json",
"x-username": "colorkite10",
...options?.headers
},

});
if (res.ok) {
return await res.json();
}

throw new Error("API 연결에 오류가 발생했습니다.");
} catch (e) {
Comment on lines +14 to +19

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번대 에러인 경우 이를 코드상 에러라고 보는 것이 맞을까요?
아니면 정상적으로 동작하는 것으로 보는게 좋을까요?
만약 정상이라고 본다면 어떻게 처리하는 것이 좋을까요?

alert(e);
push("/");
}
};
74 changes: 74 additions & 0 deletions src/components/App.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { request } from "../api/api.js";
import DocumentPage from "./DocumentPage.js";
import DocumentEditPage from "./DocumentEditPage.js";
import { initRouter } from "../routes/router.js";

export default function App({ $target }) {
this.state = {
docId: null,
docs: [],
};

this.setState = (nextState) => {
this.state = nextState;
documentPage.setState(nextState);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
documentPage.setState(nextState);
documentPage.setState(this.state);

this.statenextState 값이기 때문에 setState(this.state)가 적절할 것 같습니다!! 😁

};

const documentPage = new DocumentPage({
$target,
initialState: {
docId: null,
docs: [],
},
});

const documentEditPage = new DocumentEditPage({
$target,
initialState: {
docId: "new",
Copy link

Choose a reason for hiding this comment

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

이 new라는 부분 때문에 그런건지 모르겠는데, add a document 버튼을 누르면 id가 string 타입으로 들어가고 하위 문서 생성을 하면 id가 number 타입으로 들어가는 것 같은데요! 한 번 확인해보시면 좋을 것 같습니다!

Choose a reason for hiding this comment

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

P3;

docId의 ID가 주로 가지는 의미를 생각해보면 "중복되지 않는 유니크한 값" 이라고 생각돼요.
그랬을 때 new라는 값은 이후에 변경될 지언정 적절한지는 살짝 의문이 들었어요.

만약 id 값을 만드는게 막연하다면 uuid 같은 값을 만들어서 부여해보면 어떨까요?

doc: {
title: "",
content: "",
},
},
});

this.route = async () => {
await fetchDocuments();

const { pathname } = window.location;

if (pathname === "/") {
$target.innerHTML = "";
documentPage.render();

this.setState({ ...this.state });
} else if (pathname.indexOf("/documents/") === 0) {
const [, , docId] = pathname.split("/");

documentEditPage.setState({ docId });

this.setState({
...this.state,
docId,
});
}
};
Comment on lines +46 to +56

Choose a reason for hiding this comment

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

p2;

route의 역할을 생각해보면
"url 값에 따라 적절한 화면을 띄워준다." 가 주된 역할일 것 같아요.
그렇다보니 특정 페이지의 state를 바꾼다는 것이 살짝 어색하게 느껴져요.

만약 edit 뿐 아니라 또 다른 페이지들이 생겨난다면..? 이 안에서 if문과 state가 쌓이게 될 것 같은데 그랬을 때 가독성이 나빠질 것 같아요.

여기서는 documentEditPage를 띄우는 것 까지만 하고.
documentEditPage 내에서 init하는 과정에서 docId를 가져와 init 하는 것이 더 명확한 구분이라고 생각되는데 채연님은 어떠신가요?


const fetchDocuments = async () => {

Choose a reason for hiding this comment

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

P5;

fetch라는 의미를 생각해보면 "무언가를 가져온다" 라는 의미를 가지고 있다고 생각해요. 개발적으론 API 등에서 요청해서 가져온다 라는 의미를 가지고 있죠.

그렇다보니 fetch 함수를 호출한다고 했을 때 response 값이 return 될 것이라 기대가 되는데 여기서는 setState 도 하다보니 기대와 정말 같은 행동일까? 하는 생각이 들었어요.

사용하는 곳에서 await fetchDocuments(); 대신 const response = await fetchDocuments() 와 같이 사용하거나 initDocument() 와 같이 행동이 유추 가능한 네이밍을 해보는 것은 어떨까요?

const docs = await request("/documents", {
method: "GET",
});
Comment on lines +59 to +61
Copy link
Member

@sukvvon sukvvon Jul 10, 2023

Choose a reason for hiding this comment

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

fetch 전역 함수에서 GET 메서드의 경우 생략이 가능한걸로 알고있는데 혹시 명확성을 위해 이렇게 하신건가요? 궁금합니다!! 🤔

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({
...this.state,
docs,
});
};

this.route();

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

window.addEventListener("popstate", () => this.route());
}
168 changes: 168 additions & 0 deletions src/components/DocumentEditPage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
import Editor from "./Editor.js";
import { request } from "../api/api.js";
import { push } from "../routes/router.js";
import { getItem, setItem, removeItem } from "../utils/storage.js";

export default function DocumentEditPage({ $target, initialState }) {
const $documentEditPage = document.createElement("div");

$documentEditPage.className = "documentEditPage";

$documentEditPage.style.width = "60%";
$documentEditPage.style.margin = "auto";

this.state = initialState;

let docLocalSaveKey = `temp-document-${this.state.docId}`;
let timer = null;

Choose a reason for hiding this comment

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

P3;

이런 timer는 자주 사용되기도 하고 맥락을 명확하게 구분 지으면 좋을 것 같은데!
함수로 따로 빼보면 어떨까요?


const editor = new Editor({
$target: $documentEditPage,
initialState: {
title: "Untitled",
content: "",
},
onEditing: (doc) => {
setItem(docLocalSaveKey, {
...doc,
tempSaveDate: new Date(),

Choose a reason for hiding this comment

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

P5; ask;

제가 잘 몰라서 여쭈어봐요!!
temp를 붙이신 이유가 있을까요?
temp라는 의미는 이해가 되었는데 꼭 필요한 말이었을까? 하는 생각이 들어서요!

Copy link
Author

Choose a reason for hiding this comment

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

강의에서 임시저장값의 date를 나타내기 위해 사용한 변수명으로 알고 있습니다!

});

if (timer !== null) {
clearTimeout(timer);
}

timer = setTimeout(async () => {
const isNew = this.state.docId === "new";
if (isNew) {
if (doc.title === "") {
doc.title = "Untitled";
}

const createDocument = await request("/documents", {
method: "POST",
body: JSON.stringify(doc),
});

await request(`/documents/${createDocument.id}`, {
method: "PUT",
body: JSON.stringify(doc),
});
Comment on lines +42 to +50

Choose a reason for hiding this comment

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

ask;

생성 이후 put를 바로 하는 이유는 무엇인가요?
둘 사이에 doc데이터 변경이 없는 것 같은데 바로 put을 하는게 어색해서 여쭈어봐요!


history.replaceState(null, null, `/documents/${createDocument.id}`);
removeItem(docLocalSaveKey);

this.setState({
docId: createDocument.id,
});

push(createDocument.id);
} else {
if (doc.title === "") {
doc.title = "Untitled";
}
await request(`/documents/${doc.id}`, {
method: "PUT",
body: JSON.stringify(doc),
});

removeItem(docLocalSaveKey);
}
}, 1000);
},
});

this.setState = async (nextState) => {
if (this.state.docId === "new" && nextState.docId === "new") {
const tempDocument = await getItem(docLocalSaveKey, {
title: "Untitled",
content: "",
});

if (tempDocument.title !== "" || tempDocument.content !== "") {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (tempDocument.title !== "" || tempDocument.content !== "") {
if (!(tempDocument.title === "" && tempDocument.content === "")) {

똑같이 동작하는 코드인 것 같아서 제안드려봅니다..! 🤔

Copy link
Member

@sukvvon sukvvon Jul 10, 2023

Choose a reason for hiding this comment

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

@jgjgill 제 생각에는 OR 연산자가 더 나은 것 같습니다!! AND 연산자는 2개 다 비교해야하지만 OR 연산자 같은 경우 한 가지의 경우만 만족하면 되기 때문입니다 😀

Choose a reason for hiding this comment

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

소소한 TMI

형변환을 통한 검증도 가능할 것 같아요!

Suggested change
if (tempDocument.title !== "" || tempDocument.content !== "") {
if (!tempDocument.title || !tempDocument.content) {

this.state = {
...this.state,
doc: tempDocument,
};
} else {
this.state = nextState;
}

editor.setState(this.state.doc);

this.render();
return;
}

if (this.state.docId !== nextState.docId) {
docLocalSaveKey = `temp-document-${nextState.docId}`;
this.state = nextState;

if (this.state.docId === "new") {
const doc = getItem(docLocalSaveKey, {
title: "Untitled",
content: "",
});

editor.setState(doc);

this.render();
} else {
await fetchDocument();
}
return;
}

this.state = nextState;

this.render();

if (this.state.doc) {
editor.setState(
this.state.doc || {
title: "Untitled",
content: "",
}
);
}
};

const fetchDocument = async () => {
const { docId } = this.state;

if (this.state !== "new") {
const doc = await request(`/documents/${docId}`, {
metohd: "GET",
});

if (doc.content === null) doc.content = "";

const tempDocument = await getItem(docLocalSaveKey, {
title: "Untitled",
content: "",
});

if (
tempDocument.tempSaveDate &&
tempDocument.tempSaveDate > doc.updatedAt
) {
if (confirm("임시저장된 값을 불러올까요?")) {
this.setState({
...this.state,
doc: tempDocument,
});
return;
}
Comment on lines +149 to +155
Copy link
Member

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.

저도 임시 저장 기능을 구현한 것은 매우 좋다고 생각돼요!!! 👍

다만 위에 적은 것 처럼 fetchDocument 라는 함수에서 기대할 수 있는 행동인지는 살짝 의문이 있어요.

}

this.setState({
...this.state,
doc,
});
}
};

this.render = () => {
$target.appendChild($documentEditPage);
};
}
Loading