Skip to content

[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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rlatmdgns
Copy link

  • 요구사항을 다하지 못하였습니다. 남은 요구사항도 추가하겠습니다 ~!
  • es6 class 를 처음 사용하였고 컴포넌트화를 하는 데 미숙하여 공부가 더 필요할 것 같습니다.

🎯 요구사항

  • todo list에 todoItem을 키보드로 입력하여 추가하기
  • todo list의 체크박스를 클릭하여 complete 상태로 변경 (li tag 에 completed class 추가, input 태그에 checked 속성 추가)
  • todo list의 x버튼을 이용해서 해당 엘리먼트를 삭제
  • todo list를 더블클릭했을 때 input 모드로 변경 (li tag 에 editing class 추가) 단 이때 수정을 완료하지 않은 상태에서 esc키를 누르면 수정되지 않은 채로 다시 view 모드로 복귀
  • todo list의 item갯수를 count한 갯수를 리스트의 하단에 보여주기
  • todo list의 상태값을 확인하여, 해야할 일과, 완료한 일을 클릭하면 해당 상태의 아이템만 보여주기

🎯🎯 심화 요구사항

  • localStorage에 데이터를 저장하여, TodoItem의 CRUD를 반영하기. 따라서 새로고침하여도 저장된 데이터를 확인할 수 있어야 함

@@ -0,0 +1,14 @@
export default class TodoInput {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constructor가 빠진것같아요 !

@@ -5,16 +5,12 @@
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>이벤트 - TODOS</title>
<link rel="stylesheet" href="./src/css/style.css" />
<script type="module" src="src/index.js" defer></script>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module 방식으로 사용하시는 경우에는 defer 를 사용하실 필요없습니다! 기본적으로 defer로 작동되요 !

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer를 사용하지 않는 경우 script 로딩은 html 하단에 배치하는것이 좋습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

몰랐던 사실이네여 ㅎㅎ 감사합니다 !

set: (KEY, value) => {
window.localStorage.setItem(KEY, JSON.stringify(value))
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 아직 다 구현 못했는데 localstrage의 get과 set을 이렇게 묶어놓으니 쓰기편할것같네요 ! 참고하겠습니다 !!

init() {
this.setState(this.todoItems);
this.todoInput.setEvent({
onAdd: text => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setEvent에 들어가는 함수들을 따로 빼서 리팩토링하면 더 보기 깔끔할것같아요!

@@ -5,16 +5,12 @@
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>이벤트 - TODOS</title>
<link rel="stylesheet" href="./src/css/style.css" />
<script type="module" src="src/index.js" defer></script>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer를 사용하지 않는 경우 script 로딩은 html 하단에 배치하는것이 좋습니다.

storage.set('todo', this.todoItems);
this.todoItems = updatedItems;
this.todoList.render(this.todoItems)
this.todoCount.showCount(this.todoItems.length)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todoItems, todoList, todoCount 모두 같은 함수 이름으로 업데이트하는것도 좋을꺼 같아요!

},
});
this.todoList.render(this.todoItems)
this.todoList.setEvent({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

직접 접근해서 전달하는것보다 위의 new TodoList의 파라미터로 메소드들을 넘기는 것이 더 명확할꺼같습니다.

},
onCompleted : (event) =>{
const target = event.target;
if (!target.classList.contains('toggle')) return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이벤트위임 방식 좋아요!

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slice대신 splice를 사용하신 이유가 있나요?

Copy link
Author

@rlatmdgns rlatmdgns Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배열 자체를 수정하려고 하다가 splice 를 사용하였는데... 생각을 해보니 filter , slice가 더 적합할 것 같습니다.

this.count = document.querySelector('.todo-count strong');
}
showCount(count){
console.log(count)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아직 요구사항이 남아있긴하지만 사용하지 않는 콘솔로그는 지워주세요:)

@@ -0,0 +1,9 @@
export default class TodoCount{
constructor(){
this.count = document.querySelector('.todo-count strong');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반복되는 document.querySelector들을 util 함수로 빼서 재사용하는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

형남님 코드를 보고 선택자 utll 하나 만들어서 이렇게도 할 수 있군아. 배웠습니다 ! 감사합니다 ~

try {
return JSON.parse(window.localStorage.getItem(KEY))
} catch (e) {
return defaultData

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 || 연산자로 default를 넣었는데 try catch로 하는 방법도 괜찮은거 같네요!

Copy link

@HongJungKim-dev HongJungKim-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 잘 봤습니다 감사합니다!

todoInput.addEventListener('keydown', event => this.addTodoItem(event, onAdd));
}
addTodoItem(event, onAdd) {
const $newTodoTarget = event.target;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수명에 '$'를 하신것은 어떤 이유이신가요? 궁금해서 질문드립니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ 선택자를 사용하는 변수에는 파악하기 쉽게 $를 붙였습니다만, 좋은 방법인가 생각하는 과정에서 ? 저부분에는 잘못 표기한 것 같습니다 .

set: (KEY, value) => {
window.localStorage.setItem(KEY, JSON.stringify(value))
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POSIX에서 Line을 책정할때 한 줄 씩 읽으면서 파일 끝에 EOF가 없으면 문제가 있었다고합니다. 개행하는것은 어떤가요?
참고자료

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 자료 감사합니다 !!

this.count = document.querySelector('.todo-count strong');
}
showCount(count){
console.log(count)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

콘솔 출력은 지우셔도 좋을것 같습니다!

</li>
`
}
const template = todoItems.map((todo)=>todoItemTemplate(todo));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todoItem이 template가 된다는것이 파악하기 쉬운것 같습니다. 다만 todoItemTemplate이 함수인것 같은데 todoItem2template은 어떤가요?

const id = Number(target.closest('li').dataset.id);
console.log(id)
const item = this.todoItems.find((todoItem) => todoItem.id === id);
item.completed = !item.completed;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completed를 true false로 관리하는것 직관적이고 좋은것 같습니다.

Comment on lines +5 to +56
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)
};

this.init();
}

init() {
this.setState(this.todoItems);
this.todoInput.setEvent({
onAdd: text => {
const todo = {
id: Date.now(),
text,
completed:false,
}
this.todoItems.push(todo);
this.setState(this.todoItems);
},
});
this.todoList.render(this.todoItems)
this.todoList.setEvent({
onDelete: (event) => {
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);
this.setState(this.todoItems);
},
onCompleted : (event) =>{
const target = event.target;
if (!target.classList.contains('toggle')) return;
const id = Number(target.closest('li').dataset.id);
console.log(id)
const item = this.todoItems.find((todoItem) => todoItem.id === id);
item.completed = !item.completed;
this.setState(this.todoItems);
}
});
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스 형식으로 작성하는게 능숙하신것 같습니다. 저는 함수형식으로 주로 작성했는데 클래스 형식도 연습해봐야겠습니다.

@@ -35,18 +35,21 @@ export default class App {
this.todoList.render(this.todoItems)
this.todoList.setEvent({
onDelete: (event) => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이벤트를 각 컴포넌트에서 처리할 수 있는 건 컴포넌트에서 처리

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants