Conversation
Dom tree read Set variable
If form value submitted, then eventListner execute callback function. Callback function include: 1. preventDefault to refresh when value summitted 2. assign value variable name 'val' 3. assign 'val' object name 'obj' 4. push 'obj' to array name 'yetArr' 5. two othe function execute ( setYet, setYetLocal which will be mentioned later)
If localStorage has some value, 1. Pasrse value 2. Render that value In this case, we use two category of localStorage: [yetList,doneList]
Func1 setYetLocal include:
1. Set value in localStorage using localStorage.setItem('key','value').
In this case, should use JSON type in value of localStorage.
So we you JSON.stringify().
Func2 setValue include:
Same as Func1.
Function setYet include: 1.Create DOM node 2.Insert DOM node 3.'click' event listen which uses callback 'del(func 5)', 'done(func 6)'
Func SetDone includes: 1. Create DOM node 2. Get value in localStorage, then parse JSON into js Obj 3. Render that value in done list
Func 5 includes: 1. Delete rendering data in yetList 2. Delete localStorage data in yetList
Delete data both render and localStorage. Func 6 done includes: 1. get node event occured, then get id 2. delete render 3. delte yet in localStorage 4. add done in localStorage 5. render using setDone(func 4)
While QA, find bug: When use refresh, setDone func doesn't operate clearly. So fix them and add setDone2 to operate clearly. SetDone originally contains all function in done value, But now divide that function into two function, setDone and setDone2.
Add Todo number, done number
Build list with isDone status
Both rendering and localStorege Complete delete button
Change eventListen form => input button click
This reverts commit 150285b.
There was a problem hiding this comment.
안녕하세요 시원 님 🙌🏻
우선 정성스러운 리드미와 커밋 메세지를 세세하게 작성해 주신 점이 인상깊었습니다 👍🏻 다만 변수나 함수의 이름을 조금 더 명확하게 지어 주시면 좋을 것 같다는 생각이 듭니다.
리드미 중간에 써 주신 질문에도 답을 해 드리자면, 둘 중 어떤 게 더 적절한 방식인지 정답은 없는 것 같습니다. 저는 개인 프로젝트를 진행할 때에는 한번에 몰아서 push를 하는 편이지만 협업 시에는 기능 구현이 끝난 후 바로 push하는 식으로 진행했었습니다. 제 개인적인 생각으로는 협업 시 다른 팀원분들도 코드를 최신으로 유지해야 할 필요가 있기 때문에 기능 구현 후 push하는 방식이 더 적절해 보입니다!
구현이 안 된 부분들은 스터디 시간 전까지 최대한 구현해 보시길 바랄게요! 과제하시느라 고생 많으셨고 스터디 시간 때 뵙겠습니다~
+) 그리고 PR 제목 수정 부탁드립니다~!
| // 함수 6, delete data both render and localStorage | ||
| const done = (e) => { | ||
| const id = e.target.parentElement.id; | ||
| const text = e.target.parentElement.innerText.slice(4, -1); |
There was a problem hiding this comment.
처음에 done (할일) delete , 이렇게 할일 생성목록을 만들었고 done을 클릭하면 완료 목록으로 내려가게 만들었는데요. text 에 할 일에 해당하는 텍스트만 추출하려고 했습니다.
나중에 보니까 할일 자체를 클릭하면 완료목록으로 이동하길래 done 이라는 버튼을 지웠습니당 ㅠ 고쳐야했는데 안됐네요 감사합니다
| const yetText = 'yetList'; | ||
| const doneText = 'doneList'; | ||
| const yetBtnText = 'yetBtn'; |
There was a problem hiding this comment.
저는 이럴 경우 변수명과 문자열 값을 똑같이 설정하는 게 더 좋을 것 같습니다. 코드 리뷰하면서 이게 은근 헷갈리더라구요...!
| const yetText = 'yetList'; | |
| const doneText = 'doneList'; | |
| const yetBtnText = 'yetBtn'; | |
| const yetList = 'yetList'; | |
| const doneList = 'doneList'; | |
| const yetBtn = 'yetBtn'; |
| const temp = JSON.parse(localStorage.getItem(yetText)); | ||
| const yet = temp.filter((i) => i.id != id); | ||
| const done = temp.filter((i) => i.id == id); |
There was a problem hiding this comment.
local storage에 doneList도 저장해 두셨는데 yetList만 가지고 필터링한 이유가 있으신가요? 진행 중인 할일 목록(yetList)에 있는 항목들을 기준으로 yet과 done을 나누기 때문에 완료한 할일 목록(doneList)의 개수가 제대로 카운팅되지 않는 것 같습니다.
또한 변수명이 list를 의미하는 건지, 그 안의 항목들을 의미하는 건지 약간 모호한 느낌도 있는 것 같아요. 아래처럼 수정하면 어떨까요? 또한 iterator의 이름도 명확히 작성하는 게 좋습니다!
| const temp = JSON.parse(localStorage.getItem(yetText)); | |
| const yet = temp.filter((i) => i.id != id); | |
| const done = temp.filter((i) => i.id == id); | |
| const temp = JSON.parse(localStorage.getItem(yetText)); | |
| const yetList = temp.filter((todo) => todo.id != id); | |
| const doneList = temp.filter((todo) => todo.id == id); |
| return JSON.parse(localStorage.getItem(doneText))?.length; | ||
| }; | ||
|
|
||
| const setDone2 = () => { |
There was a problem hiding this comment.
함수의 이름이 모호한 감이 있습니다. renderDoneList 같은 이름은 어떨까요?
There was a problem hiding this comment.
네 맞아요 그게 더 좋아보여요. setDone 이라고 처음에 했다가 무슨 이유때문에 하나 더 만들었는데 함수명에 신경을 못 쓰고 아무렇게 지었네요 ㅠ
| const getTodoNum = () => { | ||
| return JSON.parse(localStorage.getItem(yetText))?.length; | ||
| }; | ||
|
|
||
| const getDoneNum = () => { | ||
| return JSON.parse(localStorage.getItem(doneText))?.length; | ||
| }; |
There was a problem hiding this comment.
개인적으로 이 함수들은 updateNum 함수와 분리할 필요가 없어 보입니다...!
| const a = getTodoNum(); | ||
| const b = getDoneNum(); | ||
| todoNum.innerText = `To Do (${a == undefined ? 0 : a})`; | ||
| doneNum.innerText = `DONE (${b == undefined ? 0 : b})`; |
There was a problem hiding this comment.
이 부분도 a, b 말고 직관적인 변수명으로 바꿔 주세요! 또한 아래처럼 작성하셔도 똑같이 동작합니다.
| const a = getTodoNum(); | |
| const b = getDoneNum(); | |
| todoNum.innerText = `To Do (${a == undefined ? 0 : a})`; | |
| doneNum.innerText = `DONE (${b == undefined ? 0 : b})`; | |
| const a = getTodoNum(); | |
| const b = getDoneNum(); | |
| todoNum.innerText = `To Do (${a})`; | |
| doneNum.innerText = `DONE (${b})`; |
S-J-Kim
left a comment
There was a problem hiding this comment.
안녕하세요 시원님
프론트엔드 멘토 김선종입니다.
코드를 구조적으로 잘 짜주셨네요. 보는 입장에서 잘 이해할 수 있었습니다. 다만 변수의 네이밍은 조금 더 명확하게 해주신다면 조금 더 보기 좋은 코드가 될 수 있을것이라 생각합니다. 그 외에는 크게 코멘트 드릴 사항이 없었고, 참고 하시면 좋을 내용들을 남겨보았습니다.
과제 하느라 고생하셨습니다
| const setYetLocal = (arr) => { | ||
| localStorage.setItem(yetText, JSON.stringify(arr)); | ||
| }; | ||
| // 함수2, setValue in localStorage | ||
| const setDoneLocal = (arr) => { | ||
| localStorage.setItem(doneText, JSON.stringify(arr)); | ||
| }; |
There was a problem hiding this comment.
const setLocalStorage = (itemType, itemList) => {
localStorage.setItem(itemType, JSON.stringify(itemList));
}
const setYetLocal = (arr) => setLocalStorage(yetText, arr)
const setDoneLocal = (arr) => setLocalStorage(doneText, arr)이렇게 리팩토링 해보면 어떨까요? 유지보수성이 개선될 것 같습니다.
| }; | ||
|
|
||
| // 함수 5, delete data both render and localStorage | ||
| const del = (e) => { |
There was a problem hiding this comment.
| const del = (e) => { | |
| const deleteTodoItem = (e) => { |
함수 이름이 너무 축약된 감이 있죠? 다른 사람이 코드를 볼 때 제목만으로도 기능을 얼추 유추할 수 있어야 합니다.
There was a problem hiding this comment.
네넵 다른 사람이 봐도 바로 감이 오도록 신경쓰도록 하겠습니다.
| const updateNum = () => { | ||
| const a = getTodoNum(); | ||
| const b = getDoneNum(); | ||
| todoNum.innerText = `To Do (${a == undefined ? 0 : a})`; | ||
| doneNum.innerText = `DONE (${b == undefined ? 0 : b})`; | ||
| }; |
There was a problem hiding this comment.
const getTodoNum = () => {
return JSON.parse(localStorage.getItem(yetText))?.length ?? 0
};getTodoNum()이라는 함수의 네이밍을 고려해볼때, 해당 함수는 항상 숫자만을 반환하는 것이 좋겠죠. 이런 경우에는 getTodoNum()을 nullish coalescing operator를 사용해 이렇게 리팩토링 하면 주현님의 코멘트 suggestion처럼 수정해 볼 수 있을 것 같습니다.
There was a problem hiding this comment.
아하.. 처음에 ?.(optional chaining) 만 사용했는데 안돼서 저런 식으로 코드를 적었는데요.
?? 를 사용하면 해결이 되는군요 감사합니다!
chaaerim
left a comment
There was a problem hiding this comment.
시원님 안녕하세요! 1주차 스터디 코드리뷰파트너를 맡게 된 김채림입니다! 😀
코드리뷰를 처음 해보기도 하고 아직 저도 부족한 부분이 많아서 조심스럽지만 작성해주신 코드 잘 봤습니다 !!
특히 로컬스토리지 적용하신 부분 보면서 반성의 시간을 가졌습니다.. 저도 빨리 적용해봐야겠어요 ㅎㅎ..
또 리드미에 작성해주신 것처럼 구조를 먼저 생각하고 구현하신 점도 인상깊었습니다!!
냅다 코드부터 작성하는 제 습관에 대해 고민해보게 되었습니다 🤣
다른 분들이 리뷰를 잘 달아주셔서 제가 크게 코멘트 드릴 점이 없었던 것 같네요! 꼼꼼하게 작성해주신 주석 덕분에 코드리뷰도 하기 좋았던 것 같습니다. 스터디 시간에 뵙겠습니다!
| #yet{ | ||
| border: 1px solid red; | ||
| } | ||
| #formDiv{ |
There was a problem hiding this comment.
#yet과 #done 부분에 overflow도 같이 설정해주시면 범위 밖으로 목록이 넘어가는 걸 방지할 수 있을 것 같습니다.
There was a problem hiding this comment.
채림님 안녕하세요 저도 채림님 코드 봤는데 너무 잘해두시고 다른 분들께서 글 많이 남기셔서
저는 많은 내용을 남겨드리지 못했네요 ㅠ 제 코드 정성스럽게 봐주셔서 감사합니당
| <div id="yet"> | ||
| <div id="formDiv"> | ||
| <form action="" id="input-form"> | ||
| <input id="input-box" type="text" required> |
There was a problem hiding this comment.
저도 required 로 입력값이 없을 경우를 처리해줘야겠네요..!!
There was a problem hiding this comment.
저도 required attribute는 알지 못했었는데, 좋은 포인트인 것 같습니다.
| const setDone = (obj) => { | ||
| const li = document.createElement('li'); | ||
| const span = document.createElement('span'); | ||
| span.innerText = obj.contents; | ||
| li.id = obj.id; | ||
| li.append(span); | ||
| doneList.append(li); | ||
| updateNum(); | ||
| }; |
There was a problem hiding this comment.
yetArr과 doneArr 를 구분해서 한번 더 클릭했을 때 다시 투두 목록으로 돌아갈 수 있도록 구현하면 좋을 것 같습니다! 그리고 여기서 delete 버튼이 같이 추가가 안되어 done에서 삭제가 불가능한 것 같은데 event를 받아서
const doneToDo=e.target.parentElement;
doneList.append(doneToDo);이렇게 delete 버튼까지 다같이 가져와서 추가하는건 어떨까 하네요..!!
| .yetBtn{ | ||
| border : none; | ||
| background : none | ||
| } |
There was a problem hiding this comment.
yetBtn 과 휴지통 img이 hover될 때 cursor: pointer;를 추가해주시면 더 직관적으로 투두를 클릭했을 때 done으로 넘어가는 것을 알 수 있을 것 같습니다 !!
| const setDone2 = () => { | ||
| const li = document.createElement('li'); | ||
| const span = document.createElement('span'); | ||
| const temp = JSON.parse(localStorage.getItem(doneText)); | ||
| temp.map((i) => { | ||
| span.innerText = i.contents; | ||
| li.id = i.id; | ||
| li.append(span); | ||
| }); | ||
| doneList.append(li); | ||
| }; |
There was a problem hiding this comment.
setDone 과 setDone2를 따로 만드신 이유가 있으실까요?! 겹치는 부분이 많아 보이는데 코드리뷰가 처음이다보니 의도대로 제가 읽지 못하는 것일까봐 여쭤봅니다!!
jdaeheon
left a comment
There was a problem hiding this comment.
안녕하세요, 코드 리뷰 맡게된 15기 정대헌입니다!
시간이 없으셔서 코드를 정리하지 못하신 것 같은 흔적이 많이 있더라구요.
여유가 있으셨다면 조금 더 구조가 잘 보였을 것 같은데 아쉬움이 있는 것 같습니다.
코드에서 리엑트의 흔적도 조금 보이는데, 저도 많이 동감하는 바입니다.
수고하셨습니다.
| <div id="yet"> | ||
| <div id="formDiv"> | ||
| <form action="" id="input-form"> | ||
| <input id="input-box" type="text" required> |
There was a problem hiding this comment.
저도 required attribute는 알지 못했었는데, 좋은 포인트인 것 같습니다.
| </div> | ||
| <div id="b"> | ||
| <div id="todoNum">To Do (0)</div> | ||
| <ul id="yet-list"> |
There was a problem hiding this comment.
반복되는 요소에 대해서는 Id보다 class로 관리하는 것도 좋을 것 같습니다!
There was a problem hiding this comment.
대신 답변을 달아드리자면, 한 개의 HTML에서 id attribute는 한 개의 엘리먼트에만 부여해야 하는것이 원칙입니다. 말 그대로 identification을 위한 속성이기 때문이죠. 브라우저 parser의 예외처리에 의해 에러가 없지만, id 중복은 원칙적으로 오류 발생입니다.
| // 함수3, render in yetList | ||
| const setYet = (obj) => { | ||
| // DOM 조작(생성) | ||
| const li = document.createElement('li'); |
There was a problem hiding this comment.
이부분 함수로 정의해서 setYet(), setDone()에 모두 활용하는 것도 좋을 것 같습니다.
| input:focus {outline:none;} | ||
| * { | ||
| box-sizing: border-box; | ||
| font-family: 'SpoqaHanSansNeo-Regular'; |
There was a problem hiding this comment.
폰트를 로드하지 못했을 때 기본 폰트를 제시해주시는 것도 좋을 것 같습니다!
| font-family: 'SpoqaHanSansNeo-Regular'; | |
| font-family: 'SpoqaHanSansNeo-Regular', sans-serif; |
[1주차 전시원 미션 제출합니다.]
안녕하세요. CEOS 15기 프론트 전시원입니다. 일주차 과제 제출드립니다.
우선 완전한 기능 구현을 못 한 상태로 제출하게 되어서 죄송합니다.
계속 시도하다 이전 커밋으로 revert 한 후 그 내용으로 제출드립니다.
배포 링크 : https://vanilla-todo-15th-delta.vercel.app/
새롭게 배운 점
부족했던 점
어딘가 이상한 점
Question to mentor
아니면 기능구현때마다 push 해야하는지 질문드리고 싶습니당..
[Key Question]
DOM 은 무엇인가요?
HTML (tag) Element를 JavaScript로 생성하는 방법은 어떤 것이 있고, 어떤 방법이 가장 적합할까요?
DOM 에서 각 요소들을 노드라고 부르는데 대표적인 노드로는 HTML tag 가 있습니다. 이 tag node 를 생성하는 방법은 다음과 같은 걸로 알고 있습니다. const div = document.createElement(‘div’)
태그에서 발생하는 Event에는 어떤 것이 있고, 어떤 방법이 가장 적합할까요?
Flexbox Layout은 무엇이며, 어떻게 사용하나요?
JavaScript가 다른 언어들에 비해 주목할만한 점에는 어떤 것들이 있나요?
코드에서 주석을 다는 바람직한 방법은 무엇일까요?