[Mission4/유지영] - Project_Notion_VanillaJS #8
Conversation
src/Page/editPage/PostEditPage.js
Outdated
| }); | ||
|
|
||
| let storageTimer = null; | ||
| let serveTimer = null; |
src/Page/editPage/PostEditPage.js
Outdated
| listUpdate(); | ||
|
|
||
| const data = await request(`/documents/${post.id}`); | ||
| markupList.setState(data); |
There was a problem hiding this comment.
저도 이 부분이 계속 마음에 걸리더라구요 ㅠㅠ 수정해보겠습니다!!
There was a problem hiding this comment.
현재 PostEditPage 컴포넌트의 setState를 보면 this.state의 postId를 항상 최신으로 유지하는 방어코드가 이미 작성이 되어 있어요! 그러니까 그냥 push() 한방만 때리면 금방 해결될 수 있을 것 같습니다.
There was a problem hiding this comment.
어떻게 해결할까 고민하고 있었는데 이런 방법이 있었다니!! 감사합니다 순요님 :)😊
src/Page/listPage/Header.js
Outdated
| this.render(); | ||
|
|
||
| $header.addEventListener("click", (e) => { | ||
| const $user = e.target.closest(".header"); |
There was a problem hiding this comment.
$user === $header 아닐까요? 조건문이 불필요한 것 같습니다
src/Page/listPage/PostList.js
Outdated
| <button name="add" class="addBtn"> + </button> | ||
| <button name="remove"class="removeBtn"> - </button> |
There was a problem hiding this comment.
button에 name attr을 다는것은 부적합해보입니다~! data attr 등을 활용하면 어떨까요?
There was a problem hiding this comment.
data 로 사용할 수 있도록 수정해봐야겠어요! ;)
src/Page/listPage/PostList.js
Outdated
| if (name) { | ||
| if (name === "remove") { | ||
| onRemove(id); | ||
| return; | ||
| } else { | ||
| onAddDocument(id, name); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
또는 return이후에 else는 의미가 없으니
if (name) {
if (name === "remove") {
onRemove(id);
return;
}
onAddDocument(id, name);
return;
}이런 early return을 생각하셔도 좋을것 같아요~!
src/Page/editPage/MarkUpList.js
Outdated
| const markUpList = (list, text) => { | ||
| text += ` | ||
| <ul> | ||
| ${list | ||
| .map( | ||
| ({ id, title, documents }) => ` | ||
| <div class='documentsTree'> | ||
| <li data-id="${id}"> | ||
| <img class="svg" src="../icon/chevron-right-solid.svg" /> | ||
| ${title} | ||
| </li> | ||
| ${documents.map((document) => markUpList([document], text)).join("")} | ||
| </div> | ||
| ` | ||
| ) | ||
| .join("")} | ||
| </ul> | ||
| `; | ||
|
|
||
| return text; | ||
| }; |
There was a problem hiding this comment.
헉 그냥 return 해도 되겠군요...왠지 더해야겠다고만 생각했었는데!! 안 더하고 return 하는 방법으로 수정해보겠습니다 😄
There was a problem hiding this comment.
아무래도 InnerHtml이 기존에 있는 Html을 삭제하니 text를 더하신것 같은데
return으로 바꾸고
innerHtml += 리턴된값
을 하시면 기존 Html값에다가 추가 됩니다!
There was a problem hiding this comment.
그런 방법으로 변경하면 좋겠네요!! insertadjacenthtml에 대해서도 공부해봐야겠습니다! 의견 감사합니다 상우님 :)
| :root { | ||
| /*color*/ | ||
| --list-text-color: rgb(116, 115, 111); | ||
| --list-backgroud-color: rgb(251, 251, 250); | ||
| --list-hover-color: rgb(218, 218, 216); | ||
| --button-hover-color: rgb(199, 199, 199); | ||
| --border-line-color: rgb(216, 216, 211); | ||
| --bold-text-color: rgb(55, 53, 47); |
There was a problem hiding this comment.
사용되는 색 정리해서 변수로 정리하신 거 너무 좋은 것 같아요! 저도 css 코드에 반영해야겠습니다👍
| import instanceCheck from "../../utils/instanceCheck.js"; | ||
|
|
||
| /**선택된 postId가 없을 때 보여주는 시작 페이지 컴포넌트 */ | ||
| export default function StartPage({ $target }) { | ||
| instanceCheck(new.target); | ||
|
|
||
| const $startPage = document.createElement("div"); | ||
| $startPage.classList.add("startPage"); | ||
|
|
||
| this.render = () => { | ||
| $target.appendChild($startPage); | ||
| $startPage.innerHTML = ` | ||
| <h1 class="startTitle">시작하기</h1> | ||
| <h3>노션 클로닝 프로젝트</h3> | ||
| <ul class="startList"> | ||
| <li> <b>+ 페이지 추가하기 버튼</b>을 눌러서 페이지를 추가하세요!</li> | ||
| <li> 페이지를 클릭해서 글을 작성해보세요.</li> | ||
| <li> <b>+ 버튼</b>을 눌러서 하위 목록을 추가해보세요!</li> | ||
| <li> <b>- 버튼</b>을 눌러서 페이지를 삭제할 수 있어요.</li> | ||
| <li> 노션 로고 또는 user 를 클릭하면 시작 화면으로 돌아올 수 있어요.</li> | ||
| </ulc> | ||
|
|
||
| `; | ||
| }; | ||
|
|
||
| this.render(); | ||
| } |
There was a problem hiding this comment.
스타트 페이지로 설명 적어준 것 너무 좋은 것 같아요! 저도 추가해야겠습니다 :)
나중에 되신다면 사용자 이름과 메일을 받는 부분도 이 페이지에 추가해서 연동하면 좋을 것 같아요!
| export default function instanceCheck(target) { | ||
| if (!target) { | ||
| throw new Error("new 연산자를 붙여주세요."); | ||
| } | ||
| } |
There was a problem hiding this comment.
이전의 투두앱에서 했던 new 방어코드를 util 함수로 만들어서 체크해 주셨네요👍
저도 리뷰 반영할 때 유틸 함수로 만들어서 추가해줘야겠네요!
|
|
||
| .editPage { | ||
| width: 100vh; | ||
| padding-top: calc(var(--padding) * 5); |
There was a problem hiding this comment.
지영님은 padding 설정을 다 calc과 변수 사용해서 해주셨네요! 통일성 있어보이고 좋은 것 같아요 :)
calc 사용하는 건 잘 몰랐었는데 지영님 코드 보면서 새롭게 배워갑니다!
There was a problem hiding this comment.
저번 Todo 앱에서도 css변수를 적극적으로 활용하시려는 모습이 인상적이었는데, 이번에도 역시 잘 활용하셨네요! 저도 활용방법을 생각해봐야겠습니다~
There was a problem hiding this comment.
같은 팀원인 양상우 입니다~
너무 잘하시고 늘 빠르게 과제 올리시는 모습이 멋져요~!~! 😎😎
팀 변경 이후에도 종종 놀러와서 리뷰 남기겠습니다 ㅎㅎ
질문에 대한 답변은
- 아무래도 현재 과제 자체가 큰 틀로 view와 edite로 나뉘다 보니 겹치는 함수가 많이 없어서 작은 단위의 중복 로직을 찾아 분리하는게 좋아 보여요!
- 겹치는 부분이나 비슷한 부분이 있다면 함수형 컴포넌트에선 프로토 타입을 이용해서 확장을 하거나(하위 함수에서 불러서 사용) class 형 컴포넌트로 extends하는 방식이 있습니다!,
- 비동기 로직을 최상위 컴포넌트로 넘겨서 post와 edit부분을 분리하는 방법도 있을것 같아요!!
- li의 기본 css가 적용되어 있어서 그런것 같습니다! resetcss를 적용후 하시면 될것 같습니다!
index.html
Outdated
| @@ -0,0 +1,20 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
There was a problem hiding this comment.
사실 지금 그렇게 신경쓸 필요는 없지만!
lang과 mata tag들을 적절하게 바꿔는것도 좋다고 하네요~!!, 저도 (mata쪽을 변경해야겠어요)
(SEO 위해서..!)
| import instanceCheck from "./utils/instanceCheck.js"; | ||
|
|
||
| export default function App({ $target }) { | ||
| instanceCheck(new.target); |
| const $postListContainer = document.createElement("div"); | ||
| const $postEditContainer = document.createElement("div"); | ||
|
|
||
| $target.appendChild($postListContainer); | ||
| $target.appendChild($postEditContainer); |
src/App.js
Outdated
| content: "", | ||
| }, | ||
| }, | ||
| listUpdate: () => { |
There was a problem hiding this comment.
주로 동사를 먼저 배치하다 보니 updateList라는 말이 어울리는것 같아요!
src/Page/editPage/Editor.js
Outdated
| this.render(); | ||
|
|
||
| $editor.addEventListener("keyup", (e) => { | ||
| const { target } = e; |
There was a problem hiding this comment.
(e) => 이부분에서 바로 ({target}) => 으로 구조 분해하는 방법도 좋을것 같아요!
src/Page/editPage/MarkUpList.js
Outdated
| const markUpList = (list, text) => { | ||
| text += ` | ||
| <ul> | ||
| ${list | ||
| .map( | ||
| ({ id, title, documents }) => ` | ||
| <div class='documentsTree'> | ||
| <li data-id="${id}"> | ||
| <img class="svg" src="../icon/chevron-right-solid.svg" /> | ||
| ${title} | ||
| </li> | ||
| ${documents.map((document) => markUpList([document], text)).join("")} | ||
| </div> | ||
| ` | ||
| ) | ||
| .join("")} | ||
| </ul> | ||
| `; | ||
|
|
||
| return text; | ||
| }; |
There was a problem hiding this comment.
아무래도 InnerHtml이 기존에 있는 Html을 삭제하니 text를 더하신것 같은데
return으로 바꾸고
innerHtml += 리턴된값
을 하시면 기존 Html값에다가 추가 됩니다!
src/Page/editPage/MarkUpList.js
Outdated
| const markUpList = (list, text) => { | ||
| text += ` | ||
| <ul> | ||
| ${list | ||
| .map( | ||
| ({ id, title, documents }) => ` | ||
| <div class='documentsTree'> | ||
| <li data-id="${id}"> | ||
| <img class="svg" src="../icon/chevron-right-solid.svg" /> | ||
| ${title} | ||
| </li> | ||
| ${documents.map((document) => markUpList([document], text)).join("")} | ||
| </div> | ||
| ` | ||
| ) | ||
| .join("")} | ||
| </ul> | ||
| `; | ||
|
|
||
| return text; | ||
| }; |
| }); | ||
|
|
||
| if (tempPost.tempSaveDate && tempPost.tempSaveDate > post.updatedAt) { | ||
| if (confirm("저장되지 않은 임시 데이터가 있습니다. 불러오시겠습니까?")) { |
There was a problem hiding this comment.
confirm의 결과값이 true false로 나오기때문에 윗 조건식에 &&이나 ||르 추가하셔도 될것 같습니다!
src/Page/listPage/PostList.js
Outdated
| if (name) { | ||
| if (name === "remove") { | ||
| onRemove(id); | ||
| return; | ||
| } else { | ||
| onAddDocument(id, name); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
또는 return이후에 else는 의미가 없으니
if (name) {
if (name === "remove") {
onRemove(id);
return;
}
onAddDocument(id, name);
return;
}이런 early return을 생각하셔도 좋을것 같아요~!
src/Page/editPage/MarkUpList.js
Outdated
| text += ` | ||
| <ul> | ||
| ${list | ||
| .map( | ||
| ({ id, title, documents }) => ` | ||
| <div class='documentsTree'> | ||
| <li data-id="${id}"> | ||
| <img class="svg" src="../icon/chevron-right-solid.svg" /> | ||
| ${title} | ||
| </li> | ||
| ${documents.map((document) => markUpList([document], text)).join("")} | ||
| </div> | ||
| ` | ||
| ) | ||
| .join("")} | ||
| </ul> | ||
| `; |
There was a problem hiding this comment.
저도 text를 더해가는 방식을 처음에 생각했었는데, 멘토님과 상우님 말씀대로 return하면서 구현해 봐야겠네요!
src/Page/editPage/PostEditPage.js
Outdated
| if (storageTimer !== null) { | ||
| clearTimeout(storageTimer); | ||
| } |
There was a problem hiding this comment.
밑에 serverSave 함수에서도 동일한 형태가 반복되는데, 함수로 빼서 argument만 받는 형태로 진행해보는 건 어떨까요? :)
There was a problem hiding this comment.
또한 함수를 선언할 땐, saveStorage와 같은 형태로 선언하는 것이 좋을 것 같습니다 :)
There was a problem hiding this comment.
debounce 함수를 따로 빼서 만들어봐야겠네요! 이름도 수정해봐야겠습니다 좋은 의견 감사합니다 지연님 😊
| import instanceCheck from "../../utils/instanceCheck.js"; | ||
|
|
||
| /**선택된 postId가 없을 때 보여주는 시작 페이지 컴포넌트 */ | ||
| export default function StartPage({ $target }) { | ||
| instanceCheck(new.target); | ||
|
|
||
| const $startPage = document.createElement("div"); | ||
| $startPage.classList.add("startPage"); | ||
|
|
||
| this.render = () => { | ||
| $target.appendChild($startPage); | ||
| $startPage.innerHTML = ` | ||
| <h1 class="startTitle">시작하기</h1> | ||
| <h3>노션 클로닝 프로젝트</h3> | ||
| <ul class="startList"> | ||
| <li> <b>+ 페이지 추가하기 버튼</b>을 눌러서 페이지를 추가하세요!</li> | ||
| <li> 페이지를 클릭해서 글을 작성해보세요.</li> | ||
| <li> <b>+ 버튼</b>을 눌러서 하위 목록을 추가해보세요!</li> | ||
| <li> <b>- 버튼</b>을 눌러서 페이지를 삭제할 수 있어요.</li> | ||
| <li> 노션 로고 또는 user 를 클릭하면 시작 화면으로 돌아올 수 있어요.</li> | ||
| </ulc> | ||
|
|
||
| `; | ||
| }; | ||
|
|
||
| this.render(); | ||
| } |
src/Page/editPage/PostEditPage.js
Outdated
| listUpdate(); | ||
|
|
||
| const data = await request(`/documents/${post.id}`); | ||
| markupList.setState(data); |
There was a problem hiding this comment.
현재 PostEditPage 컴포넌트의 setState를 보면 this.state의 postId를 항상 최신으로 유지하는 방어코드가 이미 작성이 되어 있어요! 그러니까 그냥 push() 한방만 때리면 금방 해결될 수 있을 것 같습니다.
|
|
||
| .editPage { | ||
| width: 100vh; | ||
| padding-top: calc(var(--padding) * 5); |
There was a problem hiding this comment.
저번 Todo 앱에서도 css변수를 적극적으로 활용하시려는 모습이 인상적이었는데, 이번에도 역시 잘 활용하셨네요! 저도 활용방법을 생각해봐야겠습니다~
📌 과제 설명
api 주소 추가
최상위 폴더에 하단 내용을 포함하여 apiUrl.js 파일을 추가해주세요!
👩💻 요구 사항과 구현 내용
기본 요구사항
보너스 요구사항
추가 구현사항
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
util 함수로 만들거나 컴포넌트를 쪼개는 기준이 아직 어려운 것 같습니다! 별도의 함수나 util 함수로 빼거나 컴포넌트를 따로 만들면 좋을 부분이 있는지 궁금합니다!
PostList와 MarkUPList의 구조가 흡사한데 약간 다른 부분이 있어서 컴포넌트를 따로 만들었습니다. 하나의 컴포넌트로 합치는 게 좋을 것 같은데 어떤 방법으로 합칠 수 있을지 궁금합니다!
postEditPage에서 request가 불필요하게 자주 일어나고 있는 건 아닐까 싶습니다. 개선할 수 있는 방법이 있다면 알고싶어요!
css 부분에서 리스트에 마우스를 올렸을 때 li의 배경색이 바뀌도록 하였습니다. 그런데 li의 왼쪽 빈 부분없이 hover된 li의 전체 배경색이 바뀌길 원해서 ul의 padding 값을 없앴더니 하위 documents가 반복될 때 들여쓰기 구분이 되지 않아서 다시 넣게 되었습니다ㅠ 하위 컴포넌트 깊이에 따라 들여쓰기로 구분하면서 왼쪽 padding의 빈 부분없이 배경 색을 바꾸는 방법이 있을까요?