-
Notifications
You must be signed in to change notification settings - Fork 217
[10기 김승훈] TodoList with CRUD #220
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
################ | ||
# <타입> : <제목> 의 형식으로 제목을 아래 공백줄에 작성 | ||
# 제목은 50자 이내 / 변경사항이 "무엇"인지 명확히 작성 / 끝에 마침표 금지 | ||
# 예) feat: 로그인 기능 추가 | ||
|
||
# 바로 아래 공백은 지우지 마세요 (제목과 본문의 분리를 위함) | ||
|
||
################ | ||
# 본문(구체적인 내용)을 아랫줄에 작성 | ||
# 여러 줄의 메시지를 작성할 땐 "-"로 구분 (한 줄은 72자 이내) | ||
|
||
################ | ||
# 꼬릿말(footer)을 아랫줄에 작성 (현재 커밋과 관련된 이슈 번호 추가 등) | ||
# 예) Close #7 | ||
|
||
################ | ||
# feat : 새로운 기능 추가 | ||
# fix : 버그 수정 | ||
# docs : 문서 수정 | ||
# test : 테스트 코드 추가 | ||
# refact : 코드 리팩토링 | ||
# style : 코드 의미에 영향을 주지 않는 변경사항 | ||
# chore : 빌드 부분 혹은 패키지 매니저 수정사항 | ||
# design : CSS 등 사용자 UI 디자인 변경 | ||
# etc : 기타변경 | ||
################ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"printWidth": 120, | ||
"tabWidth": 2, | ||
"singleQuote": true, | ||
"trailingComma": "all", | ||
"bracketSpacing": true, | ||
"semi": true, | ||
"useTabs": false, | ||
"arrowParens": "avoid", | ||
"endOfLine": "lf" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
import TodoInput from './components/TodoInput.js'; | ||
import TodoList from './components/TodoList.js'; | ||
import TodoCount from './components/TodoCount.js'; | ||
import { storage } from './utills/storage.js'; | ||
export default class App { | ||
constructor() { | ||
this.todoItems = storage.get('todo') || []; | ||
this.todoInput = new TodoInput(); | ||
this.todoList = new TodoList(); | ||
this.todoCount = new TodoCount(); | ||
|
||
this.setState = updatedItems => { | ||
storage.set('todo', this.todoItems); | ||
this.todoItems = updatedItems; | ||
this.todoList.render(this.todoItems) | ||
this.todoCount.showCount(this.todoItems.length) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todoItems, todoList, todoCount 모두 같은 함수 이름으로 업데이트하는것도 좋을꺼 같아요! |
||
}; | ||
|
||
this.init(); | ||
} | ||
|
||
init() { | ||
this.setState(this.todoItems); | ||
this.todoInput.setEvent({ | ||
onAdd: text => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. setEvent에 들어가는 함수들을 따로 빼서 리팩토링하면 더 보기 깔끔할것같아요! |
||
const todo = { | ||
id: Date.now(), | ||
text, | ||
completed:false, | ||
} | ||
this.todoItems.push(todo); | ||
this.setState(this.todoItems); | ||
}, | ||
}); | ||
this.todoList.render(this.todoItems) | ||
this.todoList.setEvent({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 직접 접근해서 전달하는것보다 위의 new TodoList의 파라미터로 메소드들을 넘기는 것이 더 명확할꺼같습니다. |
||
onDelete: (event) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이벤트를 각 컴포넌트에서 처리할 수 있는 건 컴포넌트에서 처리 |
||
const target = event.target; | ||
if (!target.classList.contains('destroy')) return; | ||
const id = Number(target.closest('li').dataset.id); | ||
const itemIdx = this.todoItems.findIndex((item) => item.id === id); | ||
this.todoItems.splice(itemIdx, 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. slice대신 splice를 사용하신 이유가 있나요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 배열 자체를 수정하려고 하다가 splice 를 사용하였는데... 생각을 해보니 filter , slice가 더 적합할 것 같습니다. |
||
this.setState(this.todoItems); | ||
}, | ||
onCompleted : (event) =>{ | ||
const target = event.target; | ||
if (!target.classList.contains('toggle')) return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이벤트위임 방식 좋아요! |
||
const id = Number(target.closest('li').dataset.id); | ||
console.log(id) | ||
const item = this.todoItems.find((todoItem) => todoItem.id === id); | ||
item.completed = !item.completed; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. completed를 true false로 관리하는것 직관적이고 좋은것 같습니다. |
||
this.setState(this.todoItems); | ||
} | ||
}); | ||
} | ||
} | ||
Comment on lines
+5
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 클래스 형식으로 작성하는게 능숙하신것 같습니다. 저는 함수형식으로 주로 작성했는데 클래스 형식도 연습해봐야겠습니다. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
export default class TodoCount{ | ||
constructor(){ | ||
this.count = document.querySelector('.todo-count strong'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 반복되는 document.querySelector들을 util 함수로 빼서 재사용하는건 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 형남님 코드를 보고 선택자 utll 하나 만들어서 이렇게도 할 수 있군아. 배웠습니다 ! 감사합니다 ~ |
||
} | ||
showCount(count){ | ||
console.log(count) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 콘솔 출력은 지우셔도 좋을것 같습니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아직 요구사항이 남아있긴하지만 사용하지 않는 콘솔로그는 지워주세요:) |
||
this.count.innerHTML = count; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
export default class TodoInput { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. constructor가 빠진것같아요 ! |
||
setEvent({onAdd}){ | ||
const todoInput = document.querySelector('#new-todo-title'); | ||
todoInput.addEventListener('keydown', event => this.addTodoItem(event, onAdd)); | ||
} | ||
addTodoItem(event, onAdd) { | ||
const $newTodoTarget = event.target; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 변수명에 '$'를 하신것은 어떤 이유이신가요? 궁금해서 질문드립니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. $ 선택자를 사용하는 변수에는 파악하기 쉽게 $를 붙였습니다만, 좋은 방법인가 생각하는 과정에서 ? 저부분에는 잘못 표기한 것 같습니다 . |
||
if (event.key === 'Enter') { | ||
if ($newTodoTarget.value === '') return alert('할일을 입력하세여.'); | ||
onAdd($newTodoTarget.value); | ||
$newTodoTarget.value = ''; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
export default class TodoList { | ||
constructor() { | ||
this.todoList = document.querySelector('#todo-list'); | ||
} | ||
render(todoItems){ | ||
const todoItemTemplate = (todo) => { | ||
return ` | ||
<li data-id="${todo.id}" class="${todo.completed ? 'completed' : ''}"> | ||
<div class="view"> | ||
<input class="toggle" type="checkbox" ${todo.completed ? 'checked' : ''} /> | ||
<label class="label">${todo.text}</label> | ||
<button class="destroy"></button> | ||
</div> | ||
<input class="edit" value=${todo.text} /> | ||
</li> | ||
` | ||
} | ||
const template = todoItems.map((todo)=>todoItemTemplate(todo)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todoItem이 template가 된다는것이 파악하기 쉬운것 같습니다. 다만 todoItemTemplate이 함수인것 같은데 todoItem2template은 어떤가요? |
||
this.todoList.innerHTML = template.join(""); | ||
} | ||
setEvent({onDelete,onCompleted}){ | ||
this.todoList.addEventListener('click', onDelete); | ||
this.todoList.addEventListener('click', onCompleted); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import App from './App.js'; | ||
|
||
new App(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
export const storage = { | ||
get: (KEY, defaultData = []) => { | ||
try { | ||
return JSON.parse(window.localStorage.getItem(KEY)) | ||
} catch (e) { | ||
return defaultData | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저는 || 연산자로 default를 넣었는데 try catch로 하는 방법도 괜찮은거 같네요! |
||
} | ||
}, | ||
set: (KEY, value) => { | ||
window.localStorage.setItem(KEY, JSON.stringify(value)) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저는 아직 다 구현 못했는데 localstrage의 get과 set을 이렇게 묶어놓으니 쓰기편할것같네요 ! 참고하겠습니다 !! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. POSIX에서 Line을 책정할때 한 줄 씩 읽으면서 파일 끝에 EOF가 없으면 문제가 있었다고합니다. 개행하는것은 어떤가요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 자료 감사합니다 !! |
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.
module 방식으로 사용하시는 경우에는 defer 를 사용하실 필요없습니다! 기본적으로 defer로 작동되요 !
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.
defer를 사용하지 않는 경우 script 로딩은 html 하단에 배치하는것이 좋습니다.
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.
몰랐던 사실이네여 ㅎㅎ 감사합니다 !