-
Notifications
You must be signed in to change notification settings - Fork 28
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 #15
base: 3/#4_heojiyeon
Are you sure you want to change the base?
[Mission4/허지연] - Project_Notion_VanillaJS #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 지연님! 같은 유리팀 유지영입니다 😊
첫 번째 개인 프로젝트 너무 수고 많으셨습니다!!
직접 실행해보니 편집기가 실행되어있는 상태에서 새로고침을 하면 리스트가 사라지고 css도 사라지더라구요 ㅠ 저도 원인을 찾아보려고 했는데 못 찾았습니다 ...😥 이 부분 해결될 수 있다면 좋을 것 같아요! ><
+) 확인해보니 css가 절대경로가 아니라서 css가 날라가더라구요! 아래처럼 절대경로로 하면 css는 해결되는데 리스트는 해결이 안되네요.. ;-;
<link rel="stylesheet" href="/style.css" />
프로젝트 너무 수고 많으셨습니다! 리스트를 그리는 부분에서 많이 고민하신 흔적이 보여서 좋았어요 😊
$target.appendChild($page); | ||
}; | ||
|
||
// 해당 id url 요청 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수에 대한 주석을 추가할때는
/**
*
*/
로 추가하는 것을 권장합니다.
그래야 ide에서 hover를 했을때 주석까지 출력이 되기 때문입니다
이전 과제에 멘토님께서 주석은 위와 같은 형태로 추가하는 것을 권장한다고 하시더라구요! 주석을 //가 아닌 위와 같은 형태로 추가하면 좋을 것 같아요 :)
|
||
const isNew = this.state.postId === "new"; | ||
if (isNew) { | ||
const createdPost = createPost(post); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
새 페이지를 추가하면 아이디가 undefined로 들어와서 API 오류가 발생하는 것 같아요! onEditing에 async를 주고
const createdPost = await createPost(post);
해주면 오류 없이 해당 글의 id로 주소가 잘 호출되는 것 같습니다! 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우와 감사해요!!! :)
history.pushState(null, null, `/documents/${id}`); | ||
this.route(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
router.js에서 history.pushState를 한 후에 this.route를 호출하도록 이벤트를 만들어두셨으니까
push(`/documents/${id}`);
로 사용하는 방법도 있을 것 같아요 ! 😊
$target: $page, | ||
initialState: this.state.post, | ||
|
||
onEditing: post => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제목을 수정하면 리스트에 바로 반영이 안되고 있는 것 같아요😥 제목을 수정하고 서버에 저장이 되면 새로고침없이 리스트의 제목도 바로 바뀌게 되면 좋을 것 같습니다!!><
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프로젝트 하느라 정말 고생 많으셨습니다.
코드 보고 간단하게 리뷰 남겨봅니당.
+) 프로젝트 폴더 구분을 조금 더 명확하게 하면 이후에 프로젝트를 업데이트할 때 더 편리해질 것 같습니다!
if (!(this instanceof App)) { | ||
throw new Error("new로 생성하지 않았습니다."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 validation 체크는 다른 컴포넌트에서도 중복되니까 지난번 과제 때 했던 것처럼 별도 모듈로 빼주시면 반복을 줄일 수 있을 것 같아요!
|
||
const $linkButton = document.createElement("button"); | ||
|
||
$linkButton.setAttribute("class", this.state.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$linkbutton.className
으로 하는 것과 setAttribute('class', value);
로 하는 것의 차이가 궁금해서 조금 알아봤습니다. 기능상으로는 똑같이 동작을 하지만, setAttribute
의 경우에는 해당 DOM요소에 같은 이름의 속성이 있을 경우 덮어쓰기의 우려가 있고 무조건 해당 속성을 추가하는(같은 이름 속성이 있으면 값을 수정) 어마무시한 친구이고 또한 메소드 방식이다 보니 .className
처럼 속성으로 접근하는 것에 비해 속도상 차이가 있을 수 있다고 합니당
class 속성의 경우에는 대체로 'class' 속성을 set하기 보다는 .className으로 클래스를 수정하는 것을 권장하는 분위기이고, 또한 .className
으로 설정할 때에는 하나의 클래스에만 접근이 되기 때문에 클래스를 추가할 때에는 element.classList.add(<class 이름>)을 하는 게 더 좋다고 합니당.
classList.add() 방식은 속도상 이점을 고려했다기 보다는 안정성을 위한 옵션인 것 같아요!
참고로 classList가 반환하는 DOMTokenList 은 실시간으로 변경사항이 반영되는 live collection이라는 점도 알아두시면 좋을 것 같습니다!
아래는 제가 참고했던 문서 링크입니다.
https://quirksmode.org/dom/core/#attributes
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
순요님 코멘트대로, 클래스를 추가/삭제할때는 classList를 활용하면 좋을 것 같네요ㅎㅎ
this.render(); | ||
}; | ||
|
||
const createDocuments = ({ title, id, documents }, result) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기본값을 빈 값으로 시작하는 거라면
const createDocuments = ({title, id, documnets}, result = []) => {
// ...
}
로 하는 것은 어떻게 생각하시나요?
|
||
export const removeItem = key => { | ||
storage.removeItem(key); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 storage.js가 utils가 아닌 src에 있는 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pr전에 파일을 옮기다보니 실수가 발생했습니다 ㅜ_ㅜ 반영할 때 고쳐서 올리도록 할게요!!
|
||
const createDocuments = ({ title, id, documents }, result) => { | ||
result.push( | ||
`<li data-id=${id} class="parent-document"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-id="${id}"
콤마가 빠졌습니다ㅎㅎ
}; | ||
|
||
this.render = () => { | ||
if (!isInitialize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 이 플래그가 필요한 이유가 있을까요?
|
||
this.state = initialState; | ||
|
||
let postLocalSaveKey = `temp-post-${this.state.postId}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스토리지에서 이러한 키를 생성하는 별도 함수를 정의하는 것도 좋겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 지연님! 첫 번째 프로젝트 고생 많으셨습니다 :)
노션 아이콘도 추가해 주시고 색도 다르게 설정해 주셔서 더 예쁜 노션이었던 것 같습니다 😊
색 조합이 정말 예쁜 것 같아요~~ 몇몇 버그만 조금 수정하시면 완벽할 것 같습니당 👍
onRemove: async id => { | ||
await deletePost(id); | ||
push("/"); | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
편집기가 띄워져있는 상태에서 해당 포스트를 삭제하면 리스트가 오른쪽으로 이동되는 현상이 있더라고요..?!
그리고 삭제된 포스트가 편집기 화면에 계속 남아있어요 이부분 수정이 필요할 것 같습니다!
import { request } from "./request.js"; | ||
|
||
export const createPost = async post => { | ||
const createdPost = await request("/documents", { | ||
method: "POST", | ||
body: JSON.stringify(post), | ||
}); | ||
return createdPost; | ||
}; | ||
|
||
export const changePost = async post => { | ||
await request(`/documents/${post.id}`, { | ||
method: "PUT", | ||
body: JSON.stringify(post), | ||
}); | ||
}; | ||
|
||
export const fetchPost = async id => { | ||
const post = await request(`/documents/${id}`); | ||
return post; | ||
}; | ||
|
||
export const deletePost = async id => { | ||
await request(`/documents/${id}`, { | ||
method: "DELETE", | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api 호출하는 부분을 따로 정의해주신 것 좋네요👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
깔끔하고 좋군요!!
} | ||
|
||
body { | ||
background-color: #fff5e4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
css에서 사용되는 색들은 헷갈리지 않게 변수로 정의해서 사용하면 더 좋을 것 같아요! (지영님 방식)
font-family: serif; | ||
} | ||
|
||
body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 현재 화면에서는 리스트 보여주는 사이드바 부분에 여백이 조금 있는데
margin: 0
을 추가해 주셔서 기본 margin을 0으로 설정해 주시면 조금 더 예쁜 리스트가 될 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 지연님~! 같은팀 양상우 입니다!!
컴포넌트를 UI적으로 분리를 잘하시고 고민한 모습이 보이네요!!
SEO도 지키고, 중복되는 로직은 utils로 잘 분리하신것 같습니다 !!!
질문에 대한 답변으로는
- 이건 지영님이 피드백을 잘 주신것 같아요~!
- 컴포넌트를 분리하는 방법이 다양하긴 하지만 저같은 경우에는 최근 state단위로 나눕니다! 단순 UI를 변경하는 로직(호버, 엑티브)가 아닌 동적인 state가 필요한 경우를 최소 단위로 생각하고 나눕니다!! 이건 다 같이 토론도 해보면 좋을것 같아요~!
@@ -0,0 +1,17 @@ | |||
<!DOCTYPE html> | |||
<html lang="ko"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오.... 잘바꾸셨네요~!~!
id="input-title" | ||
type="text" | ||
name="title" | ||
style="width:500px;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인라인 스타일 보다는 css에서 관리하시는게 확장성을 고려했을때 더 좋을것 같아요~!~!
this.render(); | ||
|
||
$editor.addEventListener("keyup", e => { | ||
const { target } = e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$editor.addEventListener("keyup", ({target})=> {
으로 바로 구조분해 할당하는것도 좋을것 같아요~!
|
||
this.state = nextState; | ||
|
||
if (this.state.postId === "new") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
거꾸로 new가 아닌경우 바로 return을 시키는 early return도 좋을것 같아요~!~!
}); | ||
|
||
if (tempPost.tempSaveDate && tempPost.tempSaveDate > post.updated_at) { | ||
if (confirm("저장되지 않은 임시 데이터가 있습니다. 불러올까요?")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirm의 결과 값은 boolean이기 때문에 윗 조건식으로 옮겨 사용해보는것도 좋을것 같습니다!
<ul> | ||
${this.state | ||
.map(({ title, id, documents }) => | ||
createDocuments({ title, id, documents }, []).join("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ul을 push하지 말고 여기서 위와 같이 감싸는것도 좋을것 같습니다~!
@@ -0,0 +1,89 @@ | |||
* { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
최상단에서 한번 reset css를 적용하고 스타일을 작성하시면 더욱 편하실것 같아요~!
https://meyerweb.com/eric/tools/css/reset/
import { request } from "./request.js"; | ||
|
||
export const createPost = async post => { | ||
const createdPost = await request("/documents", { | ||
method: "POST", | ||
body: JSON.stringify(post), | ||
}); | ||
return createdPost; | ||
}; | ||
|
||
export const changePost = async post => { | ||
await request(`/documents/${post.id}`, { | ||
method: "PUT", | ||
body: JSON.stringify(post), | ||
}); | ||
}; | ||
|
||
export const fetchPost = async id => { | ||
const post = await request(`/documents/${id}`); | ||
return post; | ||
}; | ||
|
||
export const deletePost = async id => { | ||
await request(`/documents/${id}`, { | ||
method: "DELETE", | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
깔끔하고 좋군요!!
📌 과제 설명
바닐라 자바스크립트를 이용한 노션 클론 프로젝트
👩💻 요구 사항과 구현 내용
추가 구현할 예정
✅ PR 포인트 & 궁금한 점
새 페이지를 저장할 때마다 리스트에는 잘 저장되지만, api 호출 오류를 발생시키는데, 이 부분 해결이 잘 안됩니다 ㅠㅠ
코드에 잘못된 부분이나 추가할 사항이 보인다면 지적 부탁드립니다!
컴포넌트마다 명확하게 하는 일을 구분하고자 하는 의도와는 달리, 이것 저것 추가하면서 코드가 길어지고, 명확성이 떨어지는 것을 느꼈습니다. 일단 코드를 작성하고 나누려니 너무 복잡했는데, 적절한 방법이 있을까요?