[10기 이연권] TodoList with Ajax #97

dalcon10028 · 2021-07-19T07:13:42Z

🎯 요구사항

User 추가하기
User의 투두리스트 불러오기
User 삭제하기
todoItem 추가하기
todoItem 불러오기
todoItem complete하기
todoItem 삭제하기
todoItem contents 내용 수정하기
🎯🎯 심화 요구사항
데이터를 불러오기전 로딩바를 이용해, 사용자가 데이터가 불러와지고 있다는 것을 보여줍니다.
fetch api 사용하는 부분을 async await을 사용하여 리팩토링합니다.
github issue에서 라벨을 붙이는 것처럼, 우선순위에 따라서 badge를 추가합니다.
ES6 impot & export를 이용해 자바스크립트 파일을 리팩토링합니다.
🕵️‍♂️ 제약사항
User의 이름은 최소 2글자 이상이어야 합니다.
TodoItem의 콘텐츠는 최소 2글자 이상이어야 합니다.

HongJungKim-dev

전반적으로 코드를 이해하기 쉽게 작성하신것 같습니다~ 고생하셨습니다!

HongJungKim-dev · 2021-07-19T11:37:07Z

src/components/TodoList.js

+
+    this.todoList.addEventListener('change', ({ target }) => {
+      if (target.tagName !== 'SELECT') return;
+      const { id } = target.parentNode.parentNode.parentNode.dataset;


target.parentNode를 하나의 변수에 넣고 그 변수에서 다시 parentNode를 접근하는식으로 변수명으로 의미를 더 드러내는것은 어떤가요? 제가 이해를 못하는것인지 반복하여 parentNode로 접근하여 헷갈리는것 같습니다!

좀 더 좋은 id 접근 방식을 고려해봐야 될 것 같습니다 조언 감사합니다 !

hyoungnam

loading 처리 및 요구사항을 정확히 파악하고 구현하셔서 좋았습니다:)

hyoungnam · 2021-07-19T11:30:21Z

src/App.js

+      changeActiveUser: async (id) => {
+        try {
+          const { _id, name, todoList } = await userAPI.fetchUser(id);
+          this.setState({
+            ...this.state,
+            activeId: _id,
+            activeUsername: name,
+            activeTodolist: todoList,
+          });
+        } catch (error) {
+          throw new Error(error);
+        }
+      },
+      createUser: async (name) => {
+        try {
+          const newUser = await userAPI.createUser({ name });
+          this.setState({
+            ...this.state,
+            activeId: newUser._id,
+            activeUsername: newUser.name,
+            activeTodolist: newUser.todoList,
+          });
+        } catch (error) {
+          throw new Error(error);
+        }
+      },
+      deleteUser: async () => {
+        try {
+          await userAPI.deleteUser(this.state.activeId);
+          this.init();
+        } catch (error) {
+          throw new Error(error);
+        }
+      },
+    });


App이 너무 거대하기에 콜백로직부분들을 외부로 빼서 정리하는것도 좋을꺼 같습니다.

hyoungnam · 2021-07-19T11:35:13Z

src/App.js

+    const main = document.createElement('section');
+    main.className = 'main';


main 생성과 append하는 부분이 너무 떨어져 있습니다. 정적으로 변하지 않는 부분을 $target으로 해도 괜찮을꺼 같아요

hyoungnam · 2021-07-19T11:35:48Z

src/App.js

+            activeTodolist: todoList,
+          });
+        } catch (error) {
+          throw new Error(error);


throw를 하면 어디서 잡게되나요?

hyoungnam · 2021-07-19T11:36:23Z

src/App.js

+    try {
+      this.state = nextState;
+      this.todoList.setState({
+        isLoading: true,


저는 isLoading 처리를 안했는데 저도 해야겠습니닿ㅎㅎ

hyoungnam · 2021-07-19T11:38:15Z

src/apis/index.js

+    const options = {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify(body),
+    };
+    return $request(url, options);
+  },
+  put: (url, body) => {
+    const options = {
+      method: 'PUT',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify(body),
+    };


반복되는 option들도 재사용 가능하도록 고치면 좋을꺼 같아요

hyoungnam · 2021-07-19T11:38:38Z

src/apis/todolist.js

@@ -0,0 +1,31 @@
+import { $http } from './index.js';


http에 $는 어떤 의미인가요?

hyoungnam · 2021-07-19T11:39:04Z

src/components/TodoCount.js

+    this.countContainer.addEventListener('click', ({ target }) => {
+      if (!target.dataset.action) return;
+      const { action } = target.dataset;
+      console.log(action);


사용하지 않는 콘솔 로그는 지워주세요:)

hyoungnam · 2021-07-19T11:39:31Z

src/components/TodoInput.js

+    input.placeholder = '할 일을 입력해주세요';
+    input.autofocus = true;
+    input.addEventListener('keypress', ({ key, target }) => {
+      if (key !== 'Enter' || target.value < 2) return;


조건 처리 깔끔해요!

hyoungnam · 2021-07-19T11:40:34Z

src/components/TodoList.js

+    if (this.state.isLoading)
+      this.todoList.innerHTML = `
+        <li>
+            <div class="view">
+            <label class="label">
+                <div class="animated-background">
+                <div class="skel-mask-container">
+                    <div class="skel-mask"></div>
+                </div>
+                </div>
+            </label>
+            </div>
+        </li>
+        `;
+    else


if else 대신 this.state.isLoading ? ~ : ~ 이렇게 처리해도 좋을꺼 같습니다

rlatmdgns · 2021-07-19T12:08:05Z

src/apis/index.js

+  try {
+    const res = await fetch(`${BASE_URL}/${url}`, options);
+    if (!res.ok) {
+      throw new Error('서버의 상태가 이상합니다');


예외 처리가 잘 되어있는 거 같습니다 !

soonysoo

안녕하세요 연권님~ 😄
지난 주도 미션하시느라 수고많으셨습니다!ㅎㅎ

코드 보면서 많이 배워갑니당~ㅎㅎ
벌써 다음 미션이 무섭지만... 다음 주도 같이 화이팅해용 ㅠㅠ...

soonysoo · 2021-07-19T11:53:47Z

src/App.js

+      userList: [],
+      activeId: '',
+      activeUsername: '',
+      activeTodolist: [],


active 된 유저에 대해서는 하나로 묶어서 정의하면 더 가독성이 좋지 않을까요~?

soonysoo · 2021-07-19T11:58:09Z

src/apis/index.js

+    if (!res.ok) {
+      throw new Error(res.status);
+    }


res.ok일 때만 await res.json() 가 아니고 반대로 !res.ok일때 예외처리를 하는게 어떤 면에서 이점이 있나용?
제가 잘 몰라서 묻습니다 ㅠㅠ

fetch()로 부터 반환되는 Promise 객체는 HTTP error 상태를 reject하지 않아서 따로 처리했습니다 !
말씀하신 아래 코드와 동일한 것 같습니다.

if (res.ok) { await res.json() } throw new Error(res.status);

https://developer.mozilla.org/ko/docs/Web/API/Fetch_API/Using_Fetch

soonysoo · 2021-07-19T12:00:14Z

src/constants/todo.js

+const ALL = 'all';
+const ACTIVE = 'active';
+const COMPLETED = 'completed';


다 따로 만드신 이유가 따로 있으신가요~?
Filter안에 ALL, ACTIVE, COMPLETE 넣어놓으면 더 가독성이 있어보일 것 같습니다 ㅎㅎ

soonysoo · 2021-07-19T12:01:54Z

src/components/TodoList.js

+    this.todoList = document.createElement('ul');
+    this.todoList.className = 'todo-list';


기존에 HTML에 있는 것 활용하시면 더 좋을 것 같아용 😄

soonysoo · 2021-07-19T12:02:36Z

src/components/TodoList.js

+    });
+
+    this.todoList.addEventListener('keypress', ({ key, target }) => {
+      if (key !== 'Enter' || target.value < 2) return;


저는 두 개 나눠서 처리했는데 한번에 처리하니까 깔끔하고 좋네요!
배워갑니당 👍

soonysoo · 2021-07-19T12:03:37Z

src/components/TodoList.js

+  }
+
+  render() {
+    if (this.state.isLoading)


loading까지 구현하시고 꼼꼼하십니당 💯

soonysoo · 2021-07-19T12:08:05Z

src/components/TodoList.js

+            <button class="destroy"></button>
+            </div>
+            <input class="edit" value="${contents}" />
+        </li>


이 부분을 활용하셔서 각각의 js로 HTML에 id를 불러오거나 classname 등록하는 함수 사용을 줄이시는 것을 추천드립니다! ㅎㅎ

soonysoo · 2021-07-19T12:09:00Z

src/components/TodoList.js

+      <option value="1">1순위</option>
+      <option value="2">2순위</option>
+    </select>`;
+  }


if else절보다 if문으로 조건을 명시하여 가독성을 높이는 것도 좋을거 같아요 ㅎㅎ

dalcon10028

내 코드 리뷰

dalcon10028 · 2021-07-26T11:51:55Z

src/App.js

+        }
+      },
+      onChange: async (id, priority) => {
+        try {


api부분에 try-catch가 작성됐기 때문에 try-catch를 빼고 api에서 에러 처리를 하거나 여기서 에러에 대한 처리를 해줘야 할 것 같습니다.

dalcon10028 · 2021-07-26T11:54:40Z

src/apis/todolist.js

+    return $http.get(`api/users/${userId}/items`);
+  },
+
+  createItem(userId, contents) {


contents에 대한 타입이나 dto가 명시되어 있거나 정의한 형태가 아닐 경우 제한하면 더 좋을 것 같습니다.

dalcon10028 · 2021-07-26T11:56:26Z

src/components/TodoList.js

+
+    this.todoList.addEventListener('click', ({ target }) => {
+      if (target.className === 'toggle') {
+        const { id } = target.parentNode.parentNode.dataset;


아래와 동일한 코드를 줄이는 게 좋을 것 같습니다.

dalcon10028 added 10 commits July 13, 2021 09:37

feat: init project

523ccf5

feat: html로 작성된 요소들 js로 옮기기

f26d8f7

feat: API 모듈 생성

e886845

feat: Title, UserList 컴포넌트 구현

d091f2f

feat: TodoInput 구현

436d2ab

feat: TodoList 렌더링과 로딩바 구현

cc1af4f

feat: 로딩바 분리

eafd720

feat: todo 수정, 모두 지우기, 우선순위 변경 구현

703d45f

feat: todo delete 구현

98c57ea

refactor: 변수명 수정, async await문에 try catch 붙이기

6132095

HongJungKim-dev reviewed Jul 19, 2021

View reviewed changes

hyoungnam reviewed Jul 19, 2021

View reviewed changes

rlatmdgns reviewed Jul 19, 2021

View reviewed changes

soonysoo reviewed Jul 19, 2021

View reviewed changes

dalcon10028 commented Jul 26, 2021

View reviewed changes

		const main = document.createElement('section');
		main.className = 'main';

		this.todoList = document.createElement('ul');
		this.todoList.className = 'todo-list';

[10기 이연권] TodoList with Ajax #97

Are you sure you want to change the base?

[10기 이연권] TodoList with Ajax #97

Uh oh!

Conversation

dalcon10028 commented Jul 19, 2021

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!

hyoungnam 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!

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!

soonysoo 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalcon10028 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants