-
Notifications
You must be signed in to change notification settings - Fork 41
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
[럿고] TodoList 상태 관리하기 미션 제출합니다 #33
base: master
Are you sure you want to change the base?
Conversation
- eslint 및 prettier 적용
…단 이때 수정을 완료하지 않은 상태에서 esc키를 누르면 수정되지 않은 채로 다시 view 모드로 복귀
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.
럿고~! 🏃 🏃♂️
프론트엔드 미션 수행하느라 너무너무너무 고생했어요!!! 👏 👏 👏
익숙치 않아서 미션 수행하느라 쉽지 않았을텐데 이렇게 참여해줘서 너무 고마워요 😄
게다가 prettier와 eslint까지 써서 깔끔한 프론트엔드 코드를 구현하려 해서 놀랬어요! 🤩 너무 멋져요!!
게다가 컴포넌트 분리까지!!!! 럿고의 프론트엔드 역량이 앞으로 정말 많이 성장할거란게 보여요 👁️
그럼 럿고의 성장💪을 위해 2가지만 피드백 해볼게요 ㅎㅎ
1. 유효성 검사하는 객체를 따로 만들기
아무래도 프론트 로직 때문에 if else같은 조건이 많을 수 밖에 없는데요 value.trim() === ""
과 같이 중복되는 유효성 검사 로직들이 반복이 되고 있어요~ validator와 같은 객체를 따로 만들어서 외부에서 재사용할 수 있는 방식으로 만든다면 럿고의 코드는 훨씬 더 협업하기 좋은 코드로 발전될꺼에요 😄
2. 중복 줄이기
백엔드에서와 마찬가지로, 프론트에서도 중복을 줄이기 위한 부단한 노력이 필요해요~ 중복되는 로직이 있다면 과감히 재사용할 수 있는 함수를 만들어저 중복되는 코드가 줄어들 수 있게 해주세요
프론트엔드 미션 진행하느라 너무 고생하셨구요!!! 앞으로 🌟 더 빛나는🌟 성장 기대할게요!
ps. 현재 todoList에 하단에 완료한일과, 해야할일 탭을 클릭했을 때 정상적으로 필터링 되지 않는 것 같던데, 이 부분은 추가해서 푸시해줄꺼죠~?
@@ -0,0 +1,18 @@ | |||
{ |
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.
👍
js/TodoApp.js
Outdated
const todoItem = { | ||
content: todoInputValue, | ||
}; | ||
await api.todoItem.create(todoItem).catch((error) => alert.log(error)); |
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.
보통 async ~ await을 사용할 때는 아래와 같이 사용해요 :)
try {
await ~
await ~
} catch (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.
만일 promise가 일반적으로 resolve한다면, await promise는 결과를 반환합니다. 하지만 rejection이 된 경우, promise는 에러를 내뱉게 됩니다. 코드 라인에 throw가 있는 것 처럼 말입니다.
감사합니다~ ㅎㅎ 잘 몰랐던 부분이엿는데, 저렇게 쓰면 안되는거군요 ㅎㅎ
js/TodoApp.js
Outdated
} | ||
|
||
onToggle(id) { | ||
api.todoItem.toggle(id).catch((error) => alert(error)); |
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.
try {
await ~
await ~
} catch (e) {
alert(error)
}
위와 같이 작성하면 error 처리를 한번에 할 수 있을 것 같아요 :)
async setState(todoItems, filter) { | ||
const todoItemsResponse = await api.todoItem.get(); | ||
if (filter === FILTER.ACTIVE) { | ||
const activeItems = this.todoItems.filter( |
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 activeItems = this.todoItems.filter(todoItem => todoItem.isCompleted)
위와 같이 간단히 작성할 수도 있어요 😄
js/api/index.js
Outdated
const todoItem = { | ||
create(todoItem) { | ||
return jsonAfterResponse( | ||
"https://todo-api.roto.codes/rutgo", |
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요청하는 주소가 반복되는데, 중복을 줄여서 하나의 변수로 관리한다면 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.
BASE_URI를 만들어서 추출했습니다!
js/TodoApp.js
Outdated
this.onCancelEdit.bind(this) | ||
); | ||
this.todoCount = new TodoCount(); | ||
this.todoFilter = new TodoFilter(this.onFilter.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.todoFilter
에 담아야 하는 이유가 있을까요? 다른데서 사용하지 않는 것 같아요 🤔
js/TodoApp.js
Outdated
} | ||
|
||
async onAdd(todoInputValue) { | ||
const todoItem = { |
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.
TodoItem
생성자 함수가 있는데 사용하지 않는 이유가 있을까요~? 🤔
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.
객체를 넘기면 빈값이 들얼가더라고요..ㅠㅠ 혹시 방법을 알려주실수 잇을까요!? ㅠㅠ
js/TodoApp.js
Outdated
this.setState(this.todoItems, filter).catch((error) => alert(error)); | ||
} | ||
|
||
async setState(todoItems, filter) { |
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에 null이나 이상한 값을 넣게 되면 어떻게 될까요? 🤔
js/TodoApp.js
Outdated
|
||
onCancelEdit() { | ||
const todoItems = this.todoItems; | ||
this.setState(todoItems).catch((error) => alert(error)); |
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.setState(this.todoItems)
로 파라미터를 넘겨줘도 괜찮지 않을까요? 🤔
js/views/TodoList.js
Outdated
} | ||
|
||
onToggleHandler(event, onToggle) { | ||
if (!event.target.classList.contains(CLASS_TYPE.TOGGLE)) { |
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.classList.contains()
와 같은 로직을 많이 사용하는데, class가 있는지 없는지 체크하는 validator 객체를 따로 만들어서 hasClass()
와 같은 메서드로 재사용하면, 협업할 때도 도움이 되고, 코드상 의미도 더 명확하게 구현할 수 있을 것 같아요 :)
안녕하세요 준! 피드백 너무 감사합니다.. ㅎㅎ |
안녕하세요 준잘! 너무 늦어서 피드백 안해주시는 거 아니시죠....?
1단계 2단계를 한번에 했는데, 1단계에서 했던 코드가 2단계 에서 지워져서 아쉽네요 ㅠㅠ
이번에 하면서
this
와class
에 대해서 많이 공부했습니다..this는 힘들더라고요... ㅠㅠ
1px
을 지적한 알트만큼 따끔한 지적 부탁드려요!항상 좋은 수업과 좋은 미션 주셔서 감사합니다~ 수고하세요!