[Mission001] todolist crud #6

s280493 · 2019-12-18T10:54:42Z

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

폴더 구조, 코드 구조를 어떻게 잡아야 하는지가 가장 어렵네요.
순수 JS로 컴포넌트를 어떻게 짜야 좋을지,
컴포넌트에는 어떤 데이터, 함수가 들어가야 할지 고민하는 과정이 길었습니다..!

StellaKim1230 · 2019-12-19T14:01:15Z

mission001/jisun/js/app.js

@@ -0,0 +1,15 @@
+import { todoListData, todoIdCount } from './store/store.js';


redux store 을 생각하시고 파일 경로를 /store/store.js 로 하신건가요?

리덕스 뿐만 아니라 vuex, mobX 등 상태 관리 라이브러리가 데이터 저장소로 store라는 이름을 쓰는 걸 생각하고 폴더명/파일명을 정했습니다. 딱히 리덕스 하나만을 염두에 두고 만든 건 아닙니다.

아하 ㅎㅎ 그렇군요!! 친절한 답변 감사합니다^^

StellaKim1230 · 2019-12-19T14:01:36Z

mission001/jisun/js/app.js

+TodoInput(todoListData);
+TodoCount(todoListData, todoCount);
+
+// 선택한 텝에 따라 랜더링 다시하기 'ㅁ'


'ㅁ' 요 이모티콘은 제거해주셔도 좋을것 같아요 ㅎㅎ

StellaKim1230 · 2019-12-19T14:02:57Z

mission001/jisun/js/components/TodoCount.js

+
+export const renderCount = (data, selector) => {
+  selector.innerHTML = `총 <strong>${data.length}</strong> 개`;
+}


end of new line을 추가해주세요. app.js 파일 같은 경우는 두줄이 추가 되었네요 ㅎ

StellaKim1230 · 2019-12-19T14:03:56Z

mission001/jisun/js/components/TodoCount.js

+  renderCount(data, todoCount);
+}
+
+export const renderCount = (data, selector) => {


여기 파라미터를 selector로 받는 다면, const todoCount = document.getElementById("todoCount"); 여기에서 todoCount도 $selector 같은 이름으로 바꿔주면 좀 더 명확해질 듯 합니다

YongHoonJJo

고생하셨습니다!! 제 생각엔 이렇게 하면 어떨까 싶은 것들에 대해 코멘트 남겨봤습니다!!

그리고 궁금한 것이 있습니다.

컴포넌트를 new 키워드를 통해 객체로 만들지 않고, 매번 함수를 호출하는 방식으로 하시는 이유가 따로 있으신가요?

코드를 보면, 예를들어 renderTodoList() 를 호출하는 코드가 몇몇 보입니다. 이 함수를 호출하게 되면, 호출할때마다 이벤트 등록이 계속에서 실행하게 될텐데, 이는 낭비이지 않을까요..??

그리고 구조 잡는 것은 저 역시 쉽지 않은 부분 이지만, 같이 스터디중인 다른 분들은 어떻게 구조를 잡았는지 살펴보면 도움이 많이 되지 않을까 생각합니다..!!

YongHoonJJo · 2019-12-19T16:39:43Z

mission001/jisun/js/components/TodoList.js

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


이 소스코드의 메인 함수인 듯한데, export 보다는 export default 로 하는 것은 어떨까요?

YongHoonJJo · 2019-12-19T16:46:56Z

mission001/jisun/js/components/TodoList.js

+}
+
+// 초기 데이터에 따라 todoList render
+export const renderTodoList = (list) => {


onEditMode() 에서는 매개변수를 data 로 네이밍을 하셨는데, 여기서는 list 라고 하셨습니다. 동일한 데이터를 전달하는데, 일관되게 네이밍을 하시는 것을 어떨까요?

YongHoonJJo · 2019-12-19T16:50:55Z

mission001/jisun/js/components/TodoList.js

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


todoList.innerHTML 가 list 의 크기만큼 호출하게 되고, 호출되는 만큼 렌더링이 이뤄질텐데, 이는 불필요하게 렌더링을 많이 하게되는게 아닌가 싶습니다. HtmlString 을 누적시켜서 만든다음에 innerHTML 을 한번만 호출하는 방식으로 진행하는 것은 어떨까요?

@YongHoonJJo 저도 용훈님이 리뷰 단 내용을 남겨놓고 싶었는데,, 어떻게 말로 잘 설명해야할지 모르던 찰나에 용훈님이 달아주셧네요 ㅎㅎ

YongHoonJJo · 2019-12-19T16:56:19Z

mission001/jisun/js/components/TodoList.js

+
+// 초기 데이터에 따라 todoList render
+export const renderTodoList = (list) => {
+  todoList.innerHTML = '';


아래 if-else 문에 의해 innerHTML 이 실행이 됩니다. 이 위치에서 todoList.innerHTML = ''; 는 없어도 되지 않을까요?

YongHoonJJo · 2019-12-19T17:00:52Z

mission001/jisun/js/components/TodoList.js

+  setTodoIdCount(todoIdCount + 1);
+  console.log(todoListData)
+
+  checkbox.addEventListener("change", e => {


onComplete(e) 함수 자체가 이벤트 핸들러인 것 같은데, checkbox.addEventListener("change", onComplete) 와 같이 하는 것은 어떨까요?

YongHoonJJo · 2019-12-19T17:12:48Z

mission001/jisun/js/components/TodoInput.js

+  input.addEventListener("keydown", e => {
+    if (e.key === "Enter") {
+      if (input.value === "") {
+        alert("값을 입력해주세요 !");


빈 문자열일 경우에 대한 예외처리를 하신 것 같은데, 그렇다면 이후 아래의 코드는 실행이 안되는게 하는 것이 맞지 않을까요?

수정했습니다! 감사합니다!

YongHoonJJo · 2019-12-19T17:24:19Z

mission001/jisun/js/components/TodoList.js

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


다른 곳에서는 e.key 를 사용하셨는데, e.keyCode 와 혼용하고 계신 이유가 따로 있으신가요?

keyCode deprecated 되었다고, 제 코드에 동준님이 리뷰를 달아주셨었습니다 ㅎ KeyboardEvent를 참고하시면 될것 같다고도 리뷰 달아주셨었습니다. ㅎㅎ

keyCode가 deprecated되었기 때문에... e.key를 사용하려 했는데요, esc는 e.key에 잡히지 않더라구요.. 그래서 어쩔 수 없이 keyCode를 사용하게 되었네요.

이왕 이렇게 된 거 e.keyCode로 전부 통일할까 고민 중입니다.

그런 이슈가 있었군요!!! 지선님 덕분에 저도 하나 알고 가네요 ㅎㅎㅎ

code 비교할때 "esc"말고 "Escape"로 찾으면 잡을 수 있을 거에요.
동준님이 링크주신 아래 사이트 가시면 확인가능해요.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode

StellaKim1230 · 2019-12-20T01:33:33Z

mission001/jisun/js/components/TodoList.js

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


@YongHoonJJo 저도 용훈님이 리뷰 단 내용을 남겨놓고 싶었는데,, 어떻게 말로 잘 설명해야할지 모르던 찰나에 용훈님이 달아주셧네요 ㅎㅎ

StellaKim1230 · 2019-12-20T01:36:53Z

mission001/jisun/js/components/TodoList.js

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


keyCode deprecated 되었다고, 제 코드에 동준님이 리뷰를 달아주셨었습니다 ㅎ KeyboardEvent를 참고하시면 될것 같다고도 리뷰 달아주셨었습니다. ㅎㅎ

StellaKim1230 · 2019-12-20T01:44:36Z

mission001/jisun/js/components/TodoList.js

+
+      if (e.keyCode === 13) {
+        if (prevValue !== editInput.value) {
+          let i = 0;


이 부분을 Javascript map 함수를 사용하여 좀 더 가독성있게 코드를 바꿔봐도 좋을 것 같습니다^^
map

StellaKim1230 · 2019-12-20T01:45:17Z

mission001/jisun/js/components/TodoList.js

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


저같은 경우는 if 문이 중첩되기 시작하면, 가독성이 안좋아지는 개인적인 느낌이 들어서 아래와 같은 문법을 선호합니다.
if (e.keyCode === 13 && prevValue !== editInput.value)

StellaKim1230 · 2019-12-20T01:47:54Z

mission001/jisun/js/util/utils.js

+
+    let selectedTodoData = [];
+
+    if (


onSelectTab 안에서 여러가지 기능을 담당하고 있는 것 같습니다. 요 부분은 해당 기능만을 위한 함수를 만들어서 따로 분리 시켜도 좋을것 같아요^^

const checkSelectTab = () => { if (selectedTab.id === status.COMPLETED || selectedTab.id === status.NEED_TODO) { for (let i = 0; i < data.length; ++i) { if (data[i].status === selectedTab.id) selectedTodoData.push(data[i]); } return renderTodoList(selectedTodoData); } return renderTodoList(data); }

이런식으로요 ㅎㅎ 함수명은 딱히 생각 안나서ㅎ, 그러면 else 문을 사용하지 않고도 return 으로 작성 가능 할 것 같군요!

StellaKim1230

코드 잘 봤습니다^^.

궁금하신것 있으시면 언제든 코멘트 남겨주세요 ㅎㅎ

StellaKim1230 · 2019-12-20T01:55:31Z

mission001/jisun/index.html

+      <div class="main">
+        <input class="toggle-all" type="checkbox" id="" />
+        <ul id="todoList" class="todo-list"></ul>
+        <!-- <ul id="todo-list" class="todo-list">


주석처리는 그냥 삭제해주셔도 좋을것 같아요 ㅎㅎ

amorfati0310

수고 많으셨어요 리뷰가 도움이 되면 좋겟네요

amorfati0310 · 2019-12-26T11:12:42Z

mission001/jisun/css/style.css

+  background-image: url("data:image/svg+xml;utf8,%3Csvg%20xmlns%3D%22http%3A//www.w3.org/2000/svg%22%20width%3D%2240%22%20height%3D%2240%22%20viewBox%3D%22-10%20-18%20100%20135%22%3E%3Ccircle%20cx%3D%2250%22%20cy%3D%2250%22%20r%3D%2250%22%20fill%3D%22none%22%20stroke%3D%22%23bddad5%22%20stroke-width%3D%223%22/%3E%3Cpath%20fill%3D%22%235dc2af%22%20d%3D%22M72%2025L42%2071%2027%2056l-4%204%2020%2020%2034-52z%22/%3E%3C/svg%3E");
+}
+
+.todo-list li label {


여유가 되시면 sass로 진행을 하셔도 좋을 것 같습니다
nested , import 등 css 로 하는 것보다 많은 장점이 있습니다 :)

앗 저 이 파일 따로 주어지는 거라고 이제 알았네요 ... 앞에 얘기 무시해주세영 ㅎㅎ;;

amorfati0310 · 2019-12-26T11:29:20Z

mission001/jisun/js/api/api.js

+};
+
+export const putData = async (username, id) => {
+  await fetch(`${URL}/${username}/${id}/toggle`, {


api 함수들의 return 값을 줘야 나중에 성공이나 에러 처리를 할 때 response 값을 파악할 수 있지 않을까요?.?

amorfati0310 · 2019-12-26T11:33:44Z

mission001/jisun/js/app.js

+import { getTodoListData } from './store/store.js';
+
+// 데이터 로드, 컴포넌트 랜더링
+getTodoListData(() => {


개인적으로는 getTodoListData 이 함수 이름이 조금 어색해보였습니다. 데이터를 가지고 오는 느낌이 아니고 뷰를 초기화 하고 뷰데이터랑 sync 를 맞춰주는 것 같아서요
ex) initViewComponents 같은 네이밍이 더 적절하지 않을까요?

amorfati0310 · 2019-12-26T11:42:46Z

mission001/jisun/js/components/TodoInput.js

+}
+
+const render = data => {
+  input.addEventListener("keydown", e => {


컴포넌트마다 render , setState 같은 이름을 가져가는게 좋은데요
이 render 함수가 계속 이벤트리스너를 setState가 발생할 때마다 중복으로 걸어주는 역할을 하지는 않나 싶습니다
이벤트 바인딩과 렌더만 하는 부분을 분리하면 어떨까요?
아 그러고보니 렌더함수가 제가 말한 의미에 렌더함수라면
TodoInput 같은 부분에서 view-data-binding 연결되는 부분이 없어서 렌더함수가 필요하지 않은 것 같긴 하네요

amorfati0310 · 2019-12-26T11:44:54Z

mission001/jisun/js/components/TodoList.js

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


각각의 버튼마다 이벤트를 걸기보다 컨테이너에서 이벤트를 걸어서 버블링을 통해서 감지하는 이벤트 전파에 대해서 알아보시면 좋을 것 같습니다 :)

amorfati0310 · 2019-12-26T11:45:55Z

mission001/jisun/js/components/TodoList.js

+};
+
+export const onComplete = e => {
+  const list = e.target.parentElement.parentElement;


혹시 dom구조가 바뀌게 되는 경우 e.target.parentElement.parentElement 말고 대안이 있을지 알아보시면 좋을 것 같습니다

amorfati0310 · 2019-12-26T11:47:13Z

mission001/jisun/js/components/TodoList.js

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


2, 27, 13 등 매직넘버들을 상수로 관리해주면 더 좋을 것 같습니다 :)

amorfati0310 · 2019-12-26T11:48:54Z

mission001/jisun/js/util/utils.js

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


array 함수를 통해서 for문을 더 가독성있게 고쳐보셔도 좋을 것 같아요 :)

ganeodolu · 2019-12-30T14:28:42Z

mission001/jisun/js/components/TodoInput.js

+  input.addEventListener("keydown", e => {
+    onAddData(e, data);
+  });
+};
+
+const onAddData = (e, data) => {
+  if (e.keyCode === 13) {


클릭이벤트에서 한글을 넣고 입력시 중복 입력이 되었었다고 저번 미팅때 들었습니다.
저도 같은 현상이 발생하여 찾아본 몇가지 경우의 수를 공유드립니다. 원인은 e.keyCode가 곧 안쓰인다고 해서 e.code로 변경 후에 생겼는데요. 아래 3가지 경우의 수를 보고 참고하시면 좋을 것 같아요.

keyup 또는 keydown 과 e.code 사용시 한글 중복입력발생,

keypress와 e.code 사용시 한글 정상입력,

keyup, keydown 상관없이 e.key 사용시 한글 정상입력되었습니다.
참고로 저는 용훈님이 추천한 e.key를 사용하는 걸로 해결하였습니다.
관련링크 https://ckbcorp.tistory.com/819

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

branch 추가

d0b6e6c

todolist

19b706d

app.js에 기능 구현

037ec63

모든 기능 구현

f0135e3

기능 구현 완료

6db0518

파일 컴포넌트로 분리 시작

b6a8cd9

컴포넌트로 나눔

bd857ec

s280493 self-assigned this Dec 18, 2019

s280493 changed the title ~~Mission001/jisun~~ [Mission001]/jisun Dec 18, 2019

s280493 changed the title ~~[Mission001]/jisun~~ [Mission001] todolist crud Dec 18, 2019

s280493 requested review from StellaKim1230, YongHoonJJo and ganeodolu December 18, 2019 12:09

StellaKim1230 reviewed Dec 19, 2019

View reviewed changes

YongHoonJJo requested changes Dec 19, 2019

View reviewed changes

StellaKim1230 reviewed Dec 20, 2019

View reviewed changes

StellaKim1230 requested changes Dec 20, 2019

View reviewed changes

cienlee and others added 3 commits December 23, 2019 10:21

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

f01ba61

get, post, delete 연결

4bbc8d0

toggle 추가

5b92667

StellaKim1230 approved these changes Dec 26, 2019

View reviewed changes

amorfati0310 reviewed Dec 26, 2019

View reviewed changes

ganeodolu reviewed Dec 30, 2019

View reviewed changes

		@@ -0,0 +1,15 @@
		import { todoListData, todoIdCount } from './store/store.js';


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

		export function TodoList (data) {

[Mission001] todolist crud #6

Are you sure you want to change the base?

[Mission001] todolist crud #6

Uh oh!

Conversation

s280493 commented Dec 18, 2019 • edited by ganeodolu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

StellaKim1230 Dec 19, 2019 • 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 left a comment • 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!

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!

StellaKim1230 Dec 20, 2019 • 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!

StellaKim1230 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

s280493 commented Dec 18, 2019 •

edited by ganeodolu

Loading

StellaKim1230 Dec 19, 2019 •

edited

Loading

YongHoonJJo left a comment •

edited

Loading

StellaKim1230 Dec 20, 2019 •

edited

Loading