[Mission002] TODOS 시스템에 비동기/온오프라인을 고려하여 업데이트하기 by s280493 · Pull Request #16 · eastjun-dev/frontend

s280493 · 2019-12-26T10:33:33Z

CURD
localStorage에 데이터 넣기 - 서버에서 데이터를 가져오기에 적용하진 않았는데요,
미션에서도 언급하신 것처럼,
오프라인 모드에서 데이터를 보여주는 용도로 추후 업데이트 할 예정입니다.

StellaKim1230

코드 잘 봤습니다 ^^
지선님 코드 보면서 제 코드도 한번 돌아 볼 수 있는 시간이었습니다!
미션2 고생하셨습니다~ 👍

StellaKim1230 · 2019-12-30T08:05:54Z

mission002/jisun/js/api/api.js

@@ -0,0 +1,34 @@
+


첫 줄은 삭제해주셔도 좋을 것 같습니다^^

StellaKim1230 · 2019-12-30T08:06:44Z

mission002/jisun/js/api/api.js

+
+export let isLoading = true;
+
+export const getData = async (username) => {


로토 컨벤션은 함수명을 getData 보다는 fetchData (즉, data를 fetch해오는 부분)으로 사용한다고 하더라구요 ㅎㅎ

StellaKim1230 · 2019-12-30T08:07:23Z

mission002/jisun/js/api/api.js

+  await fetch(`${URL}/${username}/${id}/toggle`, {
+    method: 'PUT',
+  }).then(() => {
+


then() 부분은 빈공간인데 추후를 대비해서 남겨놓으신건지 궁금합니다 ㅎㅎ

네에, 쓰이지 않을까, 싶어 만들었는데, 아마 사용하지 않을 것 같네요.
지워두겠습니다~

StellaKim1230 · 2019-12-30T08:11:48Z

mission002/jisun/js/components/TodoInput.js

+
+    const todoCount = new TodoCount(data);
+    todoCount.setState(data);
+


todoInput 컴포넌트에서 todoList render 하고, todoCount render을 담당하고 있는 것 같습니다.
각 컴포넌트가 담당하는 기능들을 더 분리시켜주면 어떨까요??^^

StellaKim1230 · 2019-12-30T08:12:48Z

mission002/jisun/js/components/TodoList.js

+
+const todoList = document.getElementById("todoList");
+
+export function TodoList(data) {


아래 함수를 arrow function을 사용하셔서 TodoList 함수도 arrow function으로 맞춰주는 것이 가독성에 더 좋을 것 같습니다.

@StellaKim1230
arrow function의 경우 prototype을 가지지 않아서 생성자 함수로 사용할 수가 없습니다~ 그래서 new 로 생성할 TodoList의 경우 function 선언식으로 하는게 맞는 것 같아요 :) 저도 한참 arrow function refactoring하다가 실수했었던 경험이 있어서 정보 공유합니다 ㅎㅎ 링크

위의 동준님 말씀대로,
new로 생성할 컴포넌트를 작성할 때, arrow function으로 만들면 안 되더라구요 !
초반에 많이 삽질했던 기억이 납니다..!

그렇군요!!! 몰랐네요 ㅠㅠ 두분 덕분에 알고 갑니다 ㅎㅎ

StellaKim1230 · 2019-12-30T08:20:03Z

mission002/jisun/js/components/TodoList.js

+  const toggles = document.getElementsByClassName("toggle");
+  const deleteBtns = document.getElementsByClassName("destroy");
+
+  for (let i = 0; i < toggles.length; ++i) {


toggle의 길이를 먼저 계산하고, 그 안에서 change 이벤트 일어났을때 onComplete 해주는 것보다는, toggle에 각 index 를 부여한 다음에 toggle된 index를 찾아서 그 부분만 onComplete 를 호출해주는 것이 더 빠를 것이라 생각이 듭니다.

그리고 toggle className 중 change 이벤트 리스너를 걸어서 onComplete 함수 호출하는 부분은 TodoList 함수 밖으로 뺴는 것이 더 좋을것 같아요^^

StellaKim1230 · 2019-12-30T08:21:41Z

mission002/jisun/js/components/TodoList.js

+          editInput.style.display = "none";
+        }
+
+        if (e.keyCode === 13) {


저희 같이 동준님 코드를 참고해봐여 ㅎㅎ

StellaKim1230 · 2019-12-30T08:22:12Z

mission002/jisun/js/components/TodoList.js

+        }
+      });
+    }
+    setTimeout(() => {


혹시 setTImeout 을 왜 넣었는지 알수있을까요??

dblclick 이벤트에 이슈가 있어서, 클릭 이벤트로 바꾸고, click 횟수가 두 번이면 더블클릭으로 인지하게 하려고 했는데요,
지금 dblclick 이벤트로 바꾸고 보니, 잘 돌아가네요....!

다시 dblclick 이벤트로 바꾸었습니다~!

StellaKim1230 · 2019-12-30T08:23:50Z

mission002/jisun/js/util/utils.js

@@ -0,0 +1,33 @@
+import { renderTodoList } from '../components/TodoList.js';
+
+export const onSelectTab = (data) => {


onSelectTag 은 전체할일, 해야할일, 완료한일 클릭시 실행되는 함수인거죠??

넵 맞습니다!

StellaKim1230 · 2019-12-30T08:25:10Z

mission002/jisun/js/util/utils.js

+    let selectedTodoData = [];
+
+    if (selectedTab.id === 'completed') {
+      for (let i = 0; i < data.length; ++i) {


filter 함수를 사용하면 코드가 더 간결해질 것 같습니다.
filter

selectedTodoData = filter(data => data.isCompleted)

이런식으로도 가능하게되면 array.push 함수를 사용 하지 않아도 될 것 같아요!

eastjun-dev

지선님 코드를 처음 피드백 드려보는 것 같습니다 :)
코드를 보다보니 3가지를 좀 더 신경써주시면 보다 협업하기 좋은 코드로 금방 변할 수 있을 것 같아요~
지선님의 노력의 시간이 보다 더 효율적으로 사용될 수 있도록 열심히 피드백으로 도와드리겠습니다 👍

필요없는 주석 제거
주석은 꼭 필요한 경우에만 남겨야 좋을 것 같아요. :)
depth 줄이기
보통 로직을 구현하다가 if문 안에 for문 안에 if문 이런식으로 depth가 2가 넘어가면 코드가 복잡해지는 경우가 많더라구요. depth가 3이상이 된다면, 해당 부분을
함수로 따로 분리해서 구현하면 보다 읽기 좋은 코드가 될 것 같아요 :)
event delegatation
현재 모든 btn element들을 for문으로 돌면서 이벤트를 바인딩하는 코드를 볼 수 있는데, 이는 event delegation을 이용하면 훨씬 간결하게 줄일 수 있어요.
혹시 이해가 가지 않거나 어려운 부분 있으시면 질문 주세요 😄

eastjun-dev · 2019-12-30T03:04:45Z

mission002/jisun/js/api/api.js

+export const putData = async (username, id) => {
+  await fetch(`${URL}/${username}/${id}/toggle`, {
+    method: 'PUT',
+  }).then(() => {


then 이후에 구현부분이 없는데, 추가할 로직이 없으시다면 제거해주셔도 좋을 것 같아요 :)

eastjun-dev · 2019-12-30T03:05:02Z

mission002/jisun/js/app.js

+import { TodoTab } from "./components/TodoTab.js";
+import { getTodoListData } from './store/store.js';
+
+// 데이터 로드, 컴포넌트 랜더링


꼭 필요한 주석이 아니라면 없어도 괜찮을 것 같아요 :)

eastjun-dev · 2019-12-30T03:07:17Z

mission002/jisun/js/components/TodoList.js

+  };
+}
+
+// 초기 데이터에 따라 todoList render


꼭 필요한 주석이 아니라면 없어도 괜찮을 것 같아요 :)

eastjun-dev · 2019-12-30T03:08:17Z

mission002/jisun/js/store/store.js

@@ -0,0 +1,36 @@
+import { getData, postData, deleteData, putData } from '../api/api.js';
+
+const username = 'jisun';


현재 자바스크립트 코드에서 double quote와 single quote가 혼용되서 사용하고 있는데 하나로 정해서 사용해주시는게 협업하는 측면에서 좋을 것 같아요 :) 그리고 이번 기회에 eslint를 적요해보시는 것도 좋을 것 같아요~ 세부적인 컨벤션 등을 신경쓰지 않아도 되서 매우 편리합니다 :)

eastjun-dev · 2019-12-30T03:08:25Z

mission002/jisun/js/components/TodoList.js

+  });
+};
+
+// TodoList의 data 제거


꼭 필요한 주석이 아니라면 없어도 괜찮을 것 같아요 :)

eastjun-dev · 2019-12-30T15:31:23Z

mission002/jisun/js/components/TodoList.js

+const onEditMode = data => {
+  let clickCount = 0;
+
+  todoList.addEventListener("click", e => {


더블 클릭이벤트를 구현하시려고 한 것 같아 보여요~
clickCount를 세는 것말고, dblclick를 활용해주시면 좋을 것 같아요 :)

eastjun-dev · 2019-12-30T15:32:05Z

mission002/jisun/js/components/TodoList.js

+
+      editInput.style.display = "block";
+      editInput.addEventListener("keydown", e => {
+        if (e.keyCode === 27) {


keyCode === 27 이 어떤 키보드 값을 나타내는지 알 수 없으므로, 따로 상수로 관리하면 더 보기 좋을 것 같아요 :)

eastjun-dev · 2019-12-30T15:32:52Z

mission002/jisun/js/components/TodoList.js

+      const label = e.target.childNodes[3];
+      const editInput = e.target.nextSibling.nextSibling;
+      const prevValue = editInput.value;
+      const id = e.target.parentElement.id;


ui에 나중에 tag가 추가되는 경우가 발생할 수도 있을 것 같은데 parentNode보다는 closest를 이용해보면 어떨까요~?

수정했습니다~~!

eastjun-dev · 2019-12-30T15:35:52Z

mission002/jisun/js/components/TodoList.js

+      editInput.style.display = "block";
+      editInput.addEventListener("keydown", e => {
+        if (e.keyCode === 27) {
+          editInput.style.display = "none";


javascript 에서 style을 지정해주기 보다는 class를 이용해주시면 더 프로그래밍적으로 다루기 좋을 것 같아요 :)
css 파일에 .hidden 이라는 class가 display: none을 해주고 있어요.
그래서 $element.classList.add('hidden') 그리고 $element.classList.remove('hidden') 를 이용해서 view를 컨트롤 할 수 있을 것 같아요~!

hidden class가 있었군요!! 넵넵!!

eastjun-dev · 2019-12-30T15:48:46Z

mission002/jisun/js/util/utils.js

+    let selectedTodoData = [];
+
+    if (selectedTab.id === 'completed') {
+      for (let i = 0; i < data.length; ++i) {


현재 로직 부분이 if문 안에, for문 안에, 또 if문이 있는 형태여서 코드가 복잡성이 높은 것 같아요.
for문을 통해서 값을 체크하는 부분을 filter method를 이용해서 구현하시면 아래처럼 보다 간결한 코드가 가능할 것 같아요~!
자바스크립트의 내장 함수를 이용하면 코딩할 때 많은 부분을 간략히 표현할 수 있어요 😄

if (selectedTab.id === 'completed') { const filteredItems = data.filter(item => item.isCompleted); renderTodoList(filteredItems); return; } if (selectedTab.id === 'needTodo') { const filteredItems = data.filter(item => !item.isCompleted); renderTodoList(filteredItems); return; } renderTodoList(data);

YongHoonJJo

이미 다른 분들이 리뷰해주시거나, Mission01 에서 리뷰드린 내용인 것 같습니다. 리뷰드린것들에 대한 응답 부탁드릴게요..ㅜㅜ

YongHoonJJo · 2020-01-07T06:28:55Z

mission002/jisun/js/api/api.js

+export const putData = async (username, id) => {
+  await fetch(`${URL}/${username}/${id}/toggle`, {
+    method: 'PUT',
+  }).then(() => {


await 과 then() 을 섞어서 사용하시는 이유가 있을까요?

then() 이후에 코드가 필요해질까봐 습관적으로 적었는데요, 필요가 없는 코드라서 지웠습니다. 감사합니다!

YongHoonJJo · 2020-01-08T10:20:58Z

mission002/jisun/js/components/TodoList.js

+    todoList.innerHTML = `<li>Todo List가 비어있습니다</li>`;
+  } else {
+    list.forEach(data => {
+      todoList.innerHTML += `<li${


Mission01 에서 리뷰 드렸었는데, 별다른 응답이 없으셨고, 여기서도 그대로 진행하신거 같은데, todoList.innerHTML을 한번만 호출하지 않고, forEach 를 통해서 여러번 호출하시는 이유가 있으신가요..? ㅜ

todoList.innerHTML = todoItems .map(todoItem => todoListTemplate(todoItem)) .join("");

data를 Foreach로 돌아서 그리는 게 익숙했는데,
vanillaJS에서 이 방식으로 하면, 브라우저를 매번 랜더링 한다는 걸 뒤늦게 알게 되었네요.

이렇게 코드를 개선했습니다.

피드백 감사드리구 넘 늦게 반영드려서 죄송해용ㅠ/ㅠ

map(todoItem => todoListTemplate(todoItem))
이부분은 전달받은 인자를 그대로 함수에 전달하기 때문에
map(todoListTemplate)
이렇게 하셔도 될 것 같아요!! 참고요..ㅎㅎ

YongHoonJJo · 2020-01-08T11:43:13Z

mission002/jisun/js/app.js

+  todoCount.setState(todoListData);
+
+  const todoTab = new TodoTab(todoListData);
+  todoTab.setState(todoListData);


객체를 생성하고, setState() 함수를 호출하고 나면 객체는 소멸이 될 것 같은데, 객체의 역할은 여기까지만 하면 되는걸까요?

s280493 and others added 10 commits December 17, 2019 08:11

branch 추가

d0b6e6c

todolist

19b706d

app.js에 기능 구현

037ec63

모든 기능 구현

f0135e3

기능 구현 완료

6db0518

파일 컴포넌트로 분리 시작

b6a8cd9

컴포넌트로 나눔

bd857ec

코드리뷰 반영 - 컴포넌트 생성 시 new로 생성

f01ba61

get, post, delete 연결

4bbc8d0

toggle 추가

5b92667

eastjun-dev assigned s280493 Dec 26, 2019

s280493 requested review from StellaKim1230, YongHoonJJo and eastjun-dev December 26, 2019 11:15

폴더 분리

f922dc4

StellaKim1230 requested changes Dec 30, 2019

View reviewed changes

eastjun-dev requested changes Dec 30, 2019

View reviewed changes

리뷰 반영

023e375

YongHoonJJo requested changes Jan 8, 2020

View reviewed changes


		export let isLoading = true;

		export const getData = async (username) => {


		const todoCount = new TodoCount(data);
		todoCount.setState(data);


		const todoList = document.getElementById("todoList");

		export function TodoList(data) {

		@@ -0,0 +1,33 @@
		import { renderTodoList } from '../components/TodoList.js';

		export const onSelectTab = (data) => {

		@@ -0,0 +1,36 @@
		import { getData, postData, deleteData, putData } from '../api/api.js';

		const username = 'jisun';

Conversation

s280493 commented Dec 26, 2019

Uh oh!

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

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!

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

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!

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

YongHoonJJo Jan 8, 2020 • 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!

YongHoonJJo Jan 8, 2020 •

edited

Loading