[10기 김승훈] TodoList with Ajax #101

imakerjun · 2021-07-19T11:10:48Z

No description provided.

kimjanghu

구현 잘봤습니다!! 한 주 동안 고생많으셨습니다 👍

kimjanghu · 2021-07-19T11:33:00Z

src/App.js

+  this.userList = new UserList({
+    userSelecteHandler: async ({ target }) => {
+      const id = target.dataset.id;
+      if (id === undefined || id === '') return;


if (!id) return 만해도 될 것 같아요

감사합니다 !

kimjanghu · 2021-07-19T11:35:18Z

src/App.js

+      const name = prompt('추가하고 싶은 이름을 입력해주세요.');
+      if (name.length < 2) {
+        alert('이름은 최소 2글자 이상이어야 합니다.');


2, prompt, alert안에 있는 것들은 모듈화, 상수화해서 사용하면 좋을 것 같아요.

kimjanghu · 2021-07-19T11:36:47Z

src/App.js

+    deleteTodo: async ({ target }) => {
+      const userId = this.currentUser._id;
+      const todoId = target.closest('li').dataset.id;
+      if (!isClassContains(target, 'destroy')) return;
+      await setDeleteTodo(userId, todoId);
+      this.render();
+    },


TodoList에서 함수를 정의하지 않고 이런식으로 함수를 정의해서 내려보내는 이유가 있을까요?
App.js가 너무 비대해지는거 같아요.

함수를 정의해서 내려보내는게 명확할 것 같다는 피드백을 받아서 이렇게 작업을 해보았습니다 !

App.js가 너무 비대해지는거 같아요.
app.js가 너무 많은 역할을 하는 것 같아요.
이거 대한 생각이 제가 짧았던 거 같네여 ㅠㅠ 좋은 피드백 ! 감사합니다.
개선해 보겠습니다

kimjanghu · 2021-07-19T11:38:49Z

src/App.js

+  this.render = async () => {
+    this.userTodoData = await getUserTodos(this.currentUser._id);
+    this.userList.render(this.users, this.currentUser);
+    this.userName.render(this.currentUser);
+    this.todoList.render(this.userTodoData);
+    this.todoCount.render(this.userTodoData);
+  };


app.js가 너무 많은 역할을 하는 것 같아요. store의 역할인것 같은데 분리하는 것도 좋아보입니다.

kimjanghu · 2021-07-19T11:40:05Z

src/api.js

+  try {
+    const response = await fetch(`${BASE_URL}/${data}`);
+    return response.json();
+  } catch (error) {
+    console.error(error);
+  }
+};


반복되는 코드가 많은 것 같습니다. request라는 함수를 만들어 사용하는 것도 좋아보여요

kimjanghu · 2021-07-19T11:41:07Z

src/api.js

+      method: 'POST',
+      headers: {
+        'content-type': 'application/json',
+      },
+      body: JSON.stringify(data),


이 부분도 따로 빼서 message를 만들면 좋을 것 같아요!

kimjanghu · 2021-07-19T11:43:31Z

src/api.js

+      },
+    });
+  } catch (error) {
+    console.error(error);


저도 못한 부분이긴 하지만, error에 대한 처리를 좀 더 명확하게 하면 좋을 것같아요! modal이라던지 alert이라던지..!!

kimjanghu · 2021-07-19T11:45:18Z

src/components/TodoCount.js

+    countContainer.addEventListener('click', clear);
+  };
+  function filterHandler(event) {
+    event.preventDefault();


submit을 사용하지 않는데 preventDefault를 사용하시는 이유가 있나요?

필터 a태그 클릭 시 href 사용되지 않게 하려고 했습니다.

용도에 맞게 버튼 태그를 사용하면 event.preventDefault(); 사용하지 않아도될 것 같네여

kimjanghu · 2021-07-19T11:47:28Z

src/components/TodoCount.js

+    const filterButtons = $all('.filters li a');
+    const [status] = event.target.classList;
+    const targetButton = $(`a.${status}`);
+    Object.keys(filterButtons).map(key => filterButtons[key].classList.remove('selected'));


전체적인 로직은 파악할 수 없지만 $all이 array형태로 return 되는데 Object.keys를 사용하는 이유가 있을까요?

filterButtons.forEach((item) => item.classList.remove('selected'))

이런식으로 하는건 어떨까요

kimjanghu · 2021-07-19T11:48:51Z

src/components/TodoList.js

+        ${
+          priority === 'NONE'
+            ? `       
+          <select class="chip select">
+            <option value="0" selected="">순위</option>
+            <option value="1">1순위</option>
+            <option value="2">2순위</option>
+          </select>
+          `
+            : `
+          <span class="chip ${priority === 'FIRST' ? 'primary' : 'secondary'}">
+          ${priority === 'FIRST' ? '1' : '2'}순위
+          </span>`
+        }


가독성이 좋지 않아보여요! 따로 함수로 빼서 작성하는 것도 여러 방법 중 하나 입니다.

HongJungKim-dev

고생많으셨습니다 많은 도움이 되었습니다!

HongJungKim-dev · 2021-07-19T12:03:18Z

src/components/TodoList.js

+  <li data-id=${_id} class=${isCompleted ? 'completed' : ''}>
+    <div class="view">
+      <input class="toggle" type="checkbox" ${isCompleted ? 'checked' : ''}>
+      <label class="label">
+        ${
+          priority === 'NONE'
+            ? `       
+          <select class="chip select">
+            <option value="0" selected="">순위</option>
+            <option value="1">1순위</option>
+            <option value="2">2순위</option>
+          </select>
+          `
+            : `
+          <span class="chip ${priority === 'FIRST' ? 'primary' : 'secondary'}">
+          ${priority === 'FIRST' ? '1' : '2'}순위
+          </span>`
+        }
+        ${contents}
+      </label>
+      <button class="destroy"></button>
+    </div>
+    <input class="edit" value="${contents}">
+  </li>
+  `;


20줄이 넘어서 읽기 힘든점이 있는것 같습니다. 큰 내용을 차지하는 태그를 함수로 빼는것은 어떤가요?

HongJungKim-dev · 2021-07-19T12:04:55Z

src/components/TodoList.js

+    todoList.addEventListener('click', deleteTodo);
+    todoList.addEventListener('click', completeToggle);
+    todoList.addEventListener('change', prioritySelect);
+    todoList.addEventListener('dblclick', editTodo);
+    todoList.addEventListener('keydown', updateTodo);


같은 동작이 반복되는것 같습니다!

HongJungKim-dev · 2021-07-19T12:05:12Z

src/components/UserController.js

+
+
+
+


불필요한 공백이 있는것 같습니다.

HongJungKim-dev · 2021-07-19T12:05:46Z

src/index.js

@@ -0,0 +1,2 @@
+import App from './App.js';
+new App().init()


개행이 필요한것 같습니다.

rlatmdgns · 2021-07-26T11:51:34Z

src/components/UserName.js

  this.render = user => {
-    const userName = document.querySelector('#user-title strong');
+    const userName = $('#user-title strong');
    if (user.name === '') return;


if(user.name === '') return;
const userName = $('#user-title strong');
실패 코드를 빠르게 리턴

rlatmdgns · 2021-07-26T11:52:54Z

src/api.js

@@ -0,0 +1,26 @@
+const BASE_URL = "https://js-todo-list-9ca3a.df.r.appspot.com/api"
+
+export const getUsersList = async() => {


config 로 따로 분리

rlatmdgns · 2021-07-26T11:54:52Z

src/api.js

+  }
+}
+
+export const setUser = async(data) => {


api.js
user, todo 분리가 필요할 것 같습니다.

rlatmdgns added 12 commits July 14, 2021 23:08

etc :prettierrc 및 커밋템플릿 설정

02e8ffd

feat : User 추가하기

83ef6f6

feat : user 추가 및 삭제

eeb1075

feat : todoItem 추가 및 삭제 하기

17263fc

feat : todoItem complete하기

8192733

feat : 우선순위에 따라서 badge를 추가

c153c0a

feat : contents 수정

d7b4926

fix : 중복 코드 수정

519e03f

fix : 변수명 오류 수정

a9d5540

feat : 필터 기능 추가

47bdf7f

feat : 모두삭제 기능 추가

21d8273

refact : 재사용성을 위한 선택자 함수 추가

7a7e2f3

imakerjun changed the title ~~Rlatmdgns94~~ [10기 김승훈] TodoList with Ajax Jul 19, 2021

kimjanghu reviewed Jul 19, 2021

View reviewed changes

HongJungKim-dev reviewed Jul 19, 2021

View reviewed changes

rlatmdgns reviewed Jul 26, 2021

View reviewed changes

		@@ -0,0 +1,2 @@
		import App from './App.js';
		new App().init() No newline at end of file

		@@ -0,0 +1,26 @@
		const BASE_URL = "https://js-todo-list-9ca3a.df.r.appspot.com/api"

		export const getUsersList = async() => {

[10기 김승훈] TodoList with Ajax #101

Are you sure you want to change the base?

[10기 김승훈] TodoList with Ajax #101

Uh oh!

Conversation

imakerjun commented Jul 19, 2021

Uh oh!

kimjanghu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlatmdgns Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HongJungKim-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rlatmdgns Jul 19, 2021 •

edited

Loading