-
Notifications
You must be signed in to change notification settings - Fork 216
[10기 이지은] TodoList with CRUD #219
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
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.
1주차 과제 너무 수고많으셨습니다!
과제 PR 했다는 것 만으로도 대단합니다!!
저도 보면서 많이 배워갑니다, 앞으로 남은 과제도 열심히 함께 해봅시다!! 😄
src/components/TodoApp.js
Outdated
const newTodoItem = new TodoItem(contents, ++id); | ||
this.todoItems.push(newTodoItem); | ||
this.setState(this.todoItems); | ||
todoCount.setState(this.todoItems); | ||
|
||
saveItems(); |
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.
OnAdd에 들어가는 이벤트는 handler 함수로 따로 선언해주면 가독성에 더 좋을 것 같아요! 😄
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.
네 안그래도 가독성이 많이 떨어져서 따로 분리했어요! 감사합니다 😀🙏
export const FilterType = Object.freeze({ | ||
all: 'all', | ||
active: 'active', | ||
completed: '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.
저는 이 방법은 생각해보지 못했었는데, 좋은 아이디어 하나 배워갑니다~!
}); | ||
|
||
export function TodoFilter({ filtering }) { | ||
this.filters = document.querySelector('.filters'); |
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.
document.querySelector를 컴포넌트로 선언하는 방식도 좋을 것 같아요! (https://edu.nextstep.camp/s/RZqgJhQO/ls/LWPmQkbP)
저도 이번에 처음으로 써봤는데, 유용한 것 같습니다!!
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,6 @@ | |||
import TodoApp from './components/TodoApp.js'; | |||
|
|||
window.addEventListener('DOMContentLoaded', () => { |
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.
DMContentloaded라는 이벤트를 활용하는 건 처음 봤네요! 혹시 그냥 html 스크립트 파일을 등록하는 것과는 무슨 차이가 있고 추가하신 이유가 있을까요?
src/components/TodoApp.js
Outdated
}, | ||
onDelete: (id) => { | ||
this.todoItems = this.todoItems.filter((item) => { | ||
return item.id !== 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.
별로 중요한 부분은 아니지만 이 부분은 오히려 return을 생략하는게 가독성이 더 좋을 것 같아요!
src/components/TodoApp.js
Outdated
this.setState(this.todoItems); | ||
todoCount.setState(this.todoItems); | ||
|
||
saveItems(); |
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.
setState을 할 때마다 saveItems가 반복되는데 setState에 saveItems를 포함시켜보면 어떨까요?
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.
setState에서 로컬스토리지 영역까지 해야하면 단일책임원칙에 위배되는 것 같은 느낌이여서요.. 안그래도 저걸 어떻게 리팩토링 해야할지 고민입니다😂
src/components/TodoApp.js
Outdated
todoCount.setState(this.todoItems); | ||
saveItems(); | ||
}, | ||
onEdit: (e, 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.
onEdit과 onEditing은 함수명이 헷갈릴 수 있으니 update과 edit같은 식으로 좀 더 구분이 되게 함수명을 바꿔주는게 좋을 것 같아요!
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.
안그래도 생각만 하고 있었는데 짚어주셔서 감사해요! 덕분에 바로 수정했습니다 👍
all: 'all', | ||
active: 'active', | ||
completed: '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.
Object.freeze()로 일종의 상수처리를 하는 방법 배워갑니다~:thumbsup:
this.removeSelectedClass = () => { | ||
this.filtersBtn.forEach((item) => { | ||
if (item.classList.contains('selected')) { | ||
item.classList.remove('selected'); |
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.
이부분은 toggle('selected')
를 써보시는게 어떨까요?
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.
selected를 제거하는 부분이라 없는 경우에는 추가가 되서 toggle을 쓸 수가 없었어요 ㅎㅎㅎ
src/components/TodoList.js
Outdated
}; | ||
|
||
this.todoList.addEventListener('click', (event) => { | ||
const li = event.target.parentNode.parentNode; |
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.
closest("li")
로 closest 메소드를 써보시면 어떨까요? 가장 가까운 조상 element를 찾아줍니다.
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.
처음 알았네요🙏 한번 적용해서 수정해볼게요
src/components/TodoList.js
Outdated
if (event.target.className === 'destroy') { | ||
onDelete(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.
event 안의 target property만 필요하실 때는
{target} => {
const li = target.parentNode;
처럼 쓸 수도 있어요!
- LocalStorage 분리 - setStatus 통합 - 인수 타입을 메서드 -> 함수로 수정
☕ 작업기간: 2021년 07월 05일 ~ 07월 06일
✍ 07.13 코드리뷰 참고하여 리팩토링 진행
🎯 요구사항
🎯🎯 심화 요구사항