[10기 김장후] TodoList with CRUD #216

kimjanghu · 2021-07-12T09:54:05Z

요구사항은 어느정도 완료했지만, 중복 코드가 너무 많은 것 같습니다. 버퍼 기간을 활용하여 리팩토링하도록 하겠습니다.

이슈

각 todo를 감싸고있는 ul tag에 이벤트를 걸어 이벤트 위임을 활용하여 check 표시를 하려했으나, 이벤트가 발생하는 경우에 todo의 갯수만큼 이벤트가 실행되는 이슈가 있었습니다. 따라서, 우선은 li tag를 querySelectorAll, forEach로 각각의 li에 이벤트를 걸었습니다. 왜 안되는 걸까요. 코드는 TodoList.js의 40~55번째 줄입니다. 같이 고민해주시면 감사하겠습니다.

🎯 요구사항

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를 반영하기. 따라서 새로고침하여도 저장된 데이터를 확인할 수 있어야 함

ChangSubKwak

제가 많이 얻어 갈게 많았던 코드 리뷰 였습니다.
많은 팁을 코드를 통해 주신 장후님께, 감사합니다. ^^

ChangSubKwak · 2021-07-12T11:22:03Z

src/utils/utils.js

+const $ = (selector) => document.querySelector(selector);
+const $$ = (selector) => document.querySelectorAll(selector);
+
+export { $, $$ };


$를 querySelector를 할당하여 사용하는 것은 보았는데,
$$를 querySelectorAll 이렇게도 사용할 수 있겠구나하고 좋은 팁 얻습니다. ^^

ChangSubKwak · 2021-07-12T11:24:26Z

index.html

 </head>

 <body>
-  <div class="todoapp">
+  <div id="app" class="todoapp">


저는 프론트엔드 공부가 부족해서 그런데, 이런식으로 해서 바닐라 JS만으로도 JS 분리를 깔끔하게 되는 것 같아요.
좋은 경험 얻어 갑니다. 👍

ChangSubKwak · 2021-07-12T11:27:48Z

src/core/Component.js

@@ -0,0 +1,16 @@
+export default class Component {
+  $target;


(질문) $로 변수명 시작하는 것이 다른 변수와 기능 상의 차이점이 있는지 아니면 이렇게 사용하는 것이 관습인지 궁금해요.

음 기능상의 차이는 없습니다! 편의상 저렇게 많이 쓰시더라구요

ChangSubKwak · 2021-07-12T11:29:39Z

src/core/Component.js

+  setState() {}
+  render() {}
+  bindEvents() {}
+}


처음부터 컴포넌트 단위로 설계를 하셔서 데이터를 저장하고, 보여주고, 컨트롤 하도록 MVC패턴에 기반해서 개발시도한 느낌이 들어요. 저도 다음에는 이렇게 시도해봐야 겠어요. 👍

ChangSubKwak · 2021-07-12T11:33:09Z

src/core/State.js

+  set(newState) {
+    this.state = newState;
+  }
+}


이건 마치, 데이터를 담는 그릇(Java에서 POJO 클래스)같은 데요.
이 질문이 여기서 하는 것이 적절한지는 모르겠는데요.
혹시나 해서 여쭐께요. Java에서는 get, set과 같은 것을 lombok의 @getter 같은 것으로 함수를 적지 않아도 되는 도구가 있는데, Javascript에서도 있는지 궁금해요.

javascript도 getter setter가 있습니다! https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Functions/get

ChangSubKwak · 2021-07-12T11:37:25Z

src/components/NewTodoInput.js

+  render() {}
+
+  bindEvents() {
+    this.$target.addEventListener('keyup', ({ key }) => {


(질문) 여기서 ({ key }) 이렇게 하는 것과 ( key ) 이렇게 하는 것은 차이가 없겠지요?
기초적일 수 있지만 여쭈어 보아요.

음 destructuring 키워드를 찾아보시면 될 것같아요! 차이는 없지만 전 가독성 측면에서 destructuring을 즐겨써요

ChangSubKwak · 2021-07-12T11:44:34Z

src/App.js

@@ -15,11 +16,18 @@ export default class App extends Component {
  mountChildren() {
    new NewTodoInput($('#new-todo-title'), {
      todoList: this.todoList,
-      onSubmitTodo: this.onSubmitTodo,
+      onSubmitTodo: this.mountTodoList.bind(this),


^^ 이렇게 바꾼 이유는 혹시 onSubmitTodo를 이 App.js 내부에서는 그려짐과 동시에 행해야 할 것이기에 명칭을 바꾼 것으로 생각됩니다만, key값은 그대로 두신 이유가 궁금해졌습니다.

props를 받는 컴포넌트의 네임을 바꾸기 귀찮아서 그대로 뒀습니다 ㅎㅎ 큰 이유는 없었어요

ChangSubKwak · 2021-07-12T11:50:42Z

src/utils/constants.js

@@ -1,10 +1,21 @@
-const TODO_BUTTONS = {
+const TODO_BUTTONS = Object.freeze({


freeze가 const만으로 안되는 부분을 해결해주기에 사용하신 거겠죠?
const만으로 충분히 불변이 보장될 것이라고 생각했어서요.

네 임의 조작을 방지하기 위해 freeze를 사용했습니다.

soonysoo

한 주동안 수고많으셨습니다 😺😺😺
기능구현 뿐만 아니라 정성스럽게 코드를 작성하신거 같아요!
섬세함에 정말 ! 💯 💯 💯 💯 💯 💯 💯

soonysoo · 2021-07-12T12:04:44Z

src/utils/constants.js

+  ACTIVE: 'active',
+  COMPLETED: 'completed',
+});
+


Object.freeze 좋은 사용방법 배워갑니다 👍

soonysoo · 2021-07-12T12:06:52Z

src/utils/utils.js

+  return JSON.parse(todoList);
+};
+
+export { $, $$, setLocalStorageItem, getLocalStorageItem };


util을 따로 분류하여 하시는 것 정말 좋아보입니다..!
안그래도 제 코드의 반복되는 부분이 많아 어떤 방법이 좋을지 찾고있었는데
앞으로 이런식으로 분류하면 되겠네요 ㅎㅎ
배워갑니다 😸 😸 😸 😸

soonysoo · 2021-07-12T12:11:14Z

src/components/NewTodoInput.js

+  addTodo(todo) {
+    const todoList = this.props.todoList.get();
+    todoList.push({
+      id: Date.now(),


개인의 todolist로 사용하는 todo프로젝트이니 now로도 id가 중복될 일은 없겠네요!

soonysoo · 2021-07-12T12:11:58Z

src/components/TodoCount.js

+import Component from '../core/component.js';
+import { FILTER_TYPES, TODO_BUTTONS } from '../utils/constants.js';
+import { $, $$ } from '../utils/utils.js';


해당 컴포넌트에서 써야하는 모듈만 import해오신 부분이 좋아보입니다!

soonysoo · 2021-07-12T12:13:47Z

src/components/TodoList.js

+    return `
+      <li class="todo ${checkTodo}" data-id=${id}>
+        <div class="view">
+          ${this.checkCheckbox(id, checked)}


궁금한점이 있습니다! 이 부분을 함수로 따로 빼서 작성하신 이유가 따로 있나요? 🇶🇦

보통 리액트의 경우에도 저런식으로 return에 직접 사용할 수 있지만, best practice를 보면 내부에 로직을 작성하기 보다는 저렇게 함수로 분리해서 하는걸 추천하더라구요! 물론 코딩 스타일 차이일거같아요. 내부에 로직을 작성해도 큰 문제는 없습니다.

soonysoo · 2021-07-12T12:16:48Z

src/components/TodoList.js

+
+export default class TodoList extends Component {
+  setState(newState) {
+    this.todoList = newState || this.props.todoList;


해당 문법에 대해 익숙지 않아서 질문드립니다..!ㅠㅠ~
혹시 7라인의 의미가 어떻게 되나요..

||, &&, ?? 연산자를 잘 사용하면 좋은데요. || 같은 경우 a || b일 때, a가 참값이라면 b를 보지않고 바로 a를 실행합니다. 반대로 a가 false값이라면 b를 실행합니다! 이런식으로 if보다는 || && 를 사용하는 것도 좋다고 들었어요

JamieShin0201

컴포넌트로 분리하신게 인상 깊었습니다 ㅎ.ㅎ
한 주간 고생 많으셨습니다^_^

JamieShin0201 · 2021-07-12T12:05:15Z

src/utils/utils.js

+  const todoList = localStorage.getItem(key);
+  return JSON.parse(todoList);


함수명과 리턴 값 (변수명?) 이 비슷하면 좋을 것 같아요.

JamieShin0201 · 2021-07-12T12:16:00Z

src/App.js

+    });
+
+    this.todoCountView = new TodoCount($('.count-container'), {
+      todoList: this.filteredTodoList.get() === null ? this.todoList.get() : this.filteredTodoList.get(),


todoList: this.filteredTodoList.get() ?? this.todoList.get(),

Nullish coalescing operator를 이용해 볼 수도 있을 것 같아요.

감사합니다! 한번 찾아보고 적용해보겠습니다!

JamieShin0201 · 2021-07-12T12:19:32Z

src/App.js

+      if (todo.id === id) {
+        todo.todo = value;
+        return todo;
+      }
+      return todo;


return을 두 번 작성하신 건 가독성 때문일까요?

네 어차피 todo.id와 id가 같은지 다른지 2가지 경우가 생기기 때문에 저런식으로 작성했습니다.

JamieShin0201 · 2021-07-12T12:23:16Z

src/components/NewTodoInput.js

+    todoList.push({
+      id: Date.now(),
+      todo,
+      checked: false,
+    });
+    this.props.todoList.set(todoList);


newTodoList = [...todoList, { id: Date.now(), todo, checked: false, }];

push로 배열의 상태를 바꾸는게 나을지 펼침연산자 이용해 새로운 리스트를 만드는게 좋을지 고민이 되네요.

펼침연산자를 사용하는게 더 좋아보입니다! (알고리즘을 제외하고) 최대한 원본배열을 건들지않고 작업하는게 유지보수나 디버깅 측면에서 유리하다고 들었습니다. 좋은 케이스 감사합니다.

JamieShin0201 · 2021-07-12T12:25:54Z

src/components/TodoCount.js

+      if (target.classList.contains(FILTER_TYPES.ALL)) {
+        this.props.filterState.set(FILTER_TYPES.ALL);
+        this.selectFilterTypeBtn(FILTER_TYPES.ALL);
+      }
+      if (target.classList.contains(FILTER_TYPES.ACTIVE)) {
+        this.props.filterState.set(FILTER_TYPES.ACTIVE);
+        this.selectFilterTypeBtn(FILTER_TYPES.ACTIVE);
+      }
+      if (target.classList.contains(FILTER_TYPES.COMPLETED)) {
+        this.props.filterState.set(FILTER_TYPES.COMPLETED);
+        this.selectFilterTypeBtn(FILTER_TYPES.COMPLETED);
+      }


세개의 if문이 비슷해 보이는데 함수로 만드는 것도 좋을 것 같아요.

네! 리펙토링 과정에서 한번 고려해보겠습니다.

kimjanghu and others added 14 commits July 6, 2021 21:34

feat: 초기 설정

0355050

feat: entry point 설정

3fda74c

feat: core component 추가

0fb7a79

feat: core state 추가

7c73ad4

feat: todo 추가

5da65a2

feat: todo data 형식 변경

1f04215

feat: WIP todoList checkbox 진행중..

86965d7

feat: Add check todo

fabc69e

chore: constants name 수정

58bb086

feat: todo 삭제

442933a

feat: 총 todo 갯수 표시

57aea54

feat: todo 수정

12a311c

feat: todo 수정 및 총 갯수 표시 추가

d69a150

feat: todo filter 적용

b4592f1

ChangSubKwak reviewed Jul 12, 2021

View reviewed changes

soonysoo reviewed Jul 12, 2021

View reviewed changes

JamieShin0201 reviewed Jul 12, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10기 김장후] TodoList with CRUD #216

[10기 김장후] TodoList with CRUD #216

kimjanghu commented Jul 12, 2021 •

edited

Loading

ChangSubKwak left a comment

ChangSubKwak Jul 12, 2021

ChangSubKwak Jul 12, 2021

ChangSubKwak Jul 12, 2021

kimjanghu Jul 19, 2021

ChangSubKwak Jul 12, 2021

ChangSubKwak Jul 12, 2021

kimjanghu Jul 19, 2021

ChangSubKwak Jul 12, 2021

kimjanghu Jul 19, 2021

ChangSubKwak Jul 12, 2021

kimjanghu Jul 19, 2021

ChangSubKwak Jul 12, 2021

kimjanghu Jul 19, 2021

soonysoo left a comment

soonysoo Jul 12, 2021

soonysoo Jul 12, 2021

soonysoo Jul 12, 2021

soonysoo Jul 12, 2021

soonysoo Jul 12, 2021

kimjanghu Jul 19, 2021

soonysoo Jul 12, 2021

kimjanghu Jul 19, 2021

JamieShin0201 left a comment

JamieShin0201 Jul 12, 2021

JamieShin0201 Jul 12, 2021 •

edited

Loading

kimjanghu Jul 19, 2021

JamieShin0201 Jul 12, 2021

kimjanghu Jul 19, 2021

JamieShin0201 Jul 12, 2021 •

edited

Loading

kimjanghu Jul 19, 2021

JamieShin0201 Jul 12, 2021

kimjanghu Jul 19, 2021

		@@ -1,10 +1,21 @@
		const TODO_BUTTONS = {
		const TODO_BUTTONS = Object.freeze({

		const todoList = localStorage.getItem(key);
		return JSON.parse(todoList);

[10기 김장후] TodoList with CRUD #216

Are you sure you want to change the base?

[10기 김장후] TodoList with CRUD #216

Conversation

kimjanghu commented Jul 12, 2021 • edited Loading

이슈

🎯 요구사항

🎯🎯 심화 요구사항

ChangSubKwak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soonysoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieShin0201 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieShin0201 Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieShin0201 Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjanghu commented Jul 12, 2021 •

edited

Loading

JamieShin0201 Jul 12, 2021 •

edited

Loading

JamieShin0201 Jul 12, 2021 •

edited

Loading