-
Notifications
You must be signed in to change notification settings - Fork 217
[10기 박은혜] TodoList with CRUD #221
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
base: main
Are you sure you want to change the base?
Conversation
- 페이지 생성 - todo 추가 - todo count 추가 - todo all, active, completed 분기 처리 중
- Storage 세팅 - Todolist active, completed 분기 처리
- Constant 처리 - toggleTodo, removeTodo => App 이동
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.
잘 봤습니다! 전체적으로 함수 활용에서 미스하신 부분들은 참고하실 만한 페이지를 링크해놓았으니 보시면서 수정하시면 좋을 것 같아요!
return item.status = "active" | ||
} else if (item.status == "active") { | ||
return item.status = "completed" | ||
} |
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(item.id===target.id)
item.status ==="completed"? item.status="active' : item.status="completed" 이렇게 줄여볼 수 있을 것 같습니다. 그리고 현재는 리턴식에 대입문이 쓰여서 리턴하려는 값이 명확하지 않습니다. 리턴을 생략할 수 있습니다.
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.todos = this.todos.filter(item => { | ||
if(item.id !== target.id) { | ||
return item | ||
} |
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.
filter()메서드는 매개변수 안에 조건식에 해당하는 배열을 리턴하기 때문에
filter(item=> item.id!==target.id)
이렇게 쓰시면 됩니다.
https://ko.javascript.info/array-methods 여길 참고해 보세요!
this.todoList.insertAdjacentHTML('beforeend', this.todoTemplate(item)) | ||
}) | ||
} | ||
|
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.
map은 배열을 반환하는 함수라 이렇게 insertAdjacentHTML처럼 쓰는 것은 정확하지 않습니다. 의도하신 부분대로라면 forEach가 맞겠네요.
set: (key, value) => { | ||
localStorage.setItem(key, JSON.stringify(value)) | ||
} | ||
} |
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.
getter와 setter는
get getstorage(todos_data){
}
set setstorage(key,value){
}
이런 식으로 콜론 없이 써야 한답니다. https://ko.javascript.info/property-accessors 여길 참고해보세요!
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.
그렇군요! 좋은 리뷰 감사합니다~~!
@@ -0,0 +1,3 @@ | |||
const $ = (selector) => document.querySelector(selector) | |||
|
|||
export default $ |
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.
모듈 활용 좋아요 👍
case STATUS_COMPLETED : | ||
this.handleMapCompletedTodo() | ||
break; | ||
} |
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.
switch문에는 default값을 선언해주는 것이 좋습니다. 이렇게 switch문을 활용할 수도 있지만 template 자체에 조건문을 포함하여 리턴하면 훨씬 깔끔해진답니다!
` ${filteredItems.map(({ id, content, isComplete }) => `
<li class="${isComplete ? "completed" :'false'} ${selectedItem===id?"editing":""}" data-id="${id}">
<div class="view">
<input class="toggle" type="checkbox" ${isComplete? "checked" : ''}/>
<label class="label">${content}</label>
<button class="destroy"></button>
</div>
<input class="edit" value="${content}"/>
</li>`).join(``)}``
이런 방식을 참고해보세요!
|
- completed 일 때 completed 클래스 추가 - active 일때 active 클래스 추가
todos: this.todos, | ||
onToggleTodo: this.handleToggleTodo.bind(this), | ||
onRemoveTodo: this.handleRemoveTodo.bind(this) | ||
}) |
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 bind를 왜 하는지?
this.onRemoveTodo(e.target) | ||
} else if(e.target.classList.contains("toggle")) { | ||
this.onToggleTodo(e.target) | ||
} |
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.
contains 말고 좀 더 좋은 방법은 없을까요?
과제 PR을 늦게 올리게 되어 1주차 수업에 코드리뷰는 받지 못하게 되었으나,..
그래도 1주차부터 PR을 놓치면 안될 것 같아서 일단 올리겠습니다!
(1주차부터 놓치면 2주차는 아예 손을 놓아버릴 것 같아서..ㅠㅠ)
앗,, PR을 적고나니 제가 미처 하지 못한 부분이 있었네요!
todo list를 더블클릭했을 때 input 모드로 변경.
ㄴ 이 부분은 제가 추후에 작업하여 커밋해보도록 하겠습니다~.~
🎯 요구사항
🎯🎯 심화 요구사항