[라빈] TodoList 상태관리 하기 미션 제출합니다. #10
Conversation
… 이때 수정을 완료하지 않은 상태에서 esc키를 누르면 수정되지 않은 채로 다시 view 모드로 복귀
There was a problem hiding this comment.
역시 라빈! 깔끔하게 코드를 작성하기 위해 노력한 게 잘보여요 👏 👏 👏
너무너무너무 고생했어요 👍 ㅎㅎ
라빈의 성장을 위해 딱 2가지 피드백을 남기려고 해요
1. 유틸성 로직 분리
enter key인지 확인하는 해당로직 같은 경우에는 util성 파일에서 재사용할 수 있게 분리한다면 훨씬 더 좋을것 같아요! 그래야 재사용하기 편하고, 코드 상에서도 흐름을 이해하고 읽기 좋은 코드가 될 수 있어요~!
2. 역할분리
지금 보면 TodoApp.js안에 굉장히 많은 로직이 들어있는데요. TodoApp.js에서만 꼭 구현해야하는 부분은 TodoApp.js에 있는 데이터(상태)를 다루는 로직들일거에요. 그 상태를 변경하는 부분만 다른 생성자에 파라미터로 넘겨주고, 구체적인 로직 구현부는 각 객체의 역할에 맞게 구현하면 좋을 것 같아요!
프론트엔드 미션 진행하느라 너무 고생하셨구요!!! 앞으로 🌟 더 빛나는🌟 성장 기대할게요!
| @@ -0,0 +1,9 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
.idea처럼 버전관리가 필요없는 IDE 설정 파일들은 깃허브에 push하지 않도록 .gitignore를 설정해주세요~!
| @@ -0,0 +1,8 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
.idea처럼 버전관리가 필요없는 IDE 설정 파일들은 깃허브에 push하지 않도록 .gitignore를 설정해주세요~!
| @@ -0,0 +1,9 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
.idea처럼 버전관리가 필요없는 IDE 설정 파일들은 깃허브에 push하지 않도록 .gitignore를 설정해주세요~!
| @@ -0,0 +1,6 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
.idea처럼 버전관리가 필요없는 IDE 설정 파일들은 깃허브에 push하지 않도록 .gitignore를 설정해주세요~!
| new TodoCount(event => { | ||
| event.preventDefault(); | ||
| const $target = event.target; | ||
| const tag = $target.getAttribute("href"); |
There was a problem hiding this comment.
href의 값을 비교하는 것보다는 해당 타겟의 명확한 class를 이용해 비교하는게 더 일반적이에요 😄
|
|
||
| export function TodoCount(showItems) { | ||
| this.number = document.querySelector("div.count-container > span > strong"); | ||
| this.allTodoItems = document.querySelector("div.count-container > ul > li:nth-child(1) > a"); |
There was a problem hiding this comment.
element를 찾기 위한 query가 길어진다는 생각이 들어요 😢
html에 class를 해당 element를 쉽게 찾을 수 있게 class를 추가해서 찾으면 더 좋을 것 같아요!
| todoInput.addEventListener(EVENT_TYPE.KEY_DOWN, event => this.addTodoItem(event)); | ||
|
|
||
| this.isValid = (event, value) => { | ||
| return (event.key === KEY_TYPE.ENTER) |
There was a problem hiding this comment.
enter키를 체크하는 로직이 반복되고 있는데요. util성 메서드를 모아놓는 파일을 따로 만들어서 isEnterKey()와 같이 재사용하면 어떨까요~? 특정 키보드 이벤트를 받는지와 같은 부분은 유틸성으로 만들어 놓으면 재사용하기 쉽고 코드도 더 간결해질 것 같아요!
| import {STATUS} from "./constants.js"; | ||
|
|
||
| export const todoItemTemplate = item => { | ||
| const isChecked = item.status === STATUS.COMPLETED ? "checked" : ""; |
| @@ -0,0 +1,23 @@ | |||
| export const STATUS = { | |||
There was a problem hiding this comment.
디렉토리 경로가 js/util인데요 util의 모음이 담기는 곳이니까 utils가 더 잘어울릴 것 같아요 :)
|
|
||
| let todoItemIndex = this.todoItems.length; | ||
|
|
||
| new TodoCount(event => { |
There was a problem hiding this comment.
TodoCount에서 TodoApp에 종속적인 this.setState만 파라미터로 넘겨주고, 나머지 구체적인 구현부는 TodoCount.js안에서 구현해주면 TodoApp이 더욱 깔끔해질 것 같은데 라빈의 생각은 어떤가요? 🤔
처음 구현해 보는 것이어서 의도하신 방향에 맞게 구현했는지 조금은 헷갈립니다...
TodoApp 에서 모든 상태를 관리하게 구현했습니다.
감사합니다!