[mission001] todolist crud #5

ganeodolu · 2019-12-18T08:35:45Z

#1 Mission001

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의 상태값을 확인하여, 해야할 일과, 완료한 일을 클릭하면 해당 상태의 아이템만 보여주기

필터버튼 테두리 유지를 구현하면서 selected가 적용되는 부분은 동준님 코드를 참조하였습니다. 필터기능 구현시 app에서 setState대신에 render메소드를 사용하여 this.data가 직접 수정되는 것을 방지하였습니다.

미션2 사용했던 제 코드와 로토님 라이브코딩시 사용했던 코드를 기반으로 작성하였습니다. 스터디했었던 난이도와 비슷하여 막막하지 않고, 미션 해결될 때마다 즐거웠습니다.
기능구현하기에도 녹록치 않아서 컴포넌트별로 기능이 잘 구분되지는 않은 것 같습니다.
다른 분들 코드 참조하면서 발전시키도록 하겠습니다.
한번 봐주시고 피드백 부탁드립니다. 😔

CSS형식에 맞게 todolist의 타겟 변경, innerHTML 태그 추가

2. 토글버튼으로 할일 completed상태변경 3. 삭제버튼으로 할일 삭제 4. 할일 더블클릭시 에디트모드 5. 할일 개수 하단에 표시

data index 위치를 toggle 태그안에서 li 태그로 변경

eastjun-dev

프로그래머스에서 배우신 부분들을 꼼꼼하게 다시 복습하시면서 구현해주셔서 저도 다시 복습하는 시간이 되었습니다
😄 너무너무 고생하셨어요~!
지금도 좋은데 제 기준에서 드릴 수 있는 피드백을 코멘트로 남겼는데요. 크게 2가지를 조금 더 신경써주시면 훨씬 좋을 것 같아요~!

1. 필요없는 주석 제거

주석은 꼭 필요한 경우가 아니면 제거해주시는게 좋아요~! 주석이 필요하다는건 코드도 읽고, 주석도 읽어야 하기 때문에 더욱 복잡하게 되는 것 같아요. 특히 협업하는 경우엔 더더욱 주석은 필요한 경우만 남겨야 서로 원활한 커뮤니케이션을 하는데 도움이 될 것 같아요 :)

2. 역할에 따른 함수 분리

1개의 함수는 1가지의 기능만 하도록 최대한 분리해야 나중에 재사용성이 훨씬 높아지고, 보다 구조적으로 설계할 수 있을 것 같아요. TodoList.js의 render함수를 보시면 templateHTML도 만들어주고, validation 체크도 하고 2가지 기능을 한번에 하는데 이런 부분들을 역할을 고민해보고 분리해서 불러오는 방식으로 만드는게 더 좋을 것 같아요 :)

mission001/ganeodolu/js/App.js

mission001/ganeodolu/js/TodoCount.js

mission001/ganeodolu/js/TodoInput.js

mission001/ganeodolu/js/TodoList.js

mission001/ganeodolu/js/TodoList.txt

eastjun-dev · 2019-12-19T04:37:00Z

mission001/origin/.vscode/settings.json

@@ -0,0 +1,3 @@
+{


보통 IDE의 설정파일은 원격 저장소에 관리하지 않아요~ .gitIgnore를 이용해서 실수로 push하지 않도록 세팅해놓으시면 훨씬 편하실 거에요 :)

생각도 안해봤었던 문제였어요. 잘 설정해두겠습니다.

.env 랑 dot 도 같이 알아보시면 좋을 것 같습니다 👍

.env는 환경설정 파일이라 이해가 가지만, dot파일은 찾아보니 그래프 관련 파일 또는 단축키 관련 파일이라고 나오는데 사용해본적이 없어서 잘 모르겠습니다. 어떤 dot파일인지 알려주실 수 있나요??

YongHoonJJo

고생하셨습니다! 저도 많이 부족한 실력이지만, 제가 가지고 있는 지식 내에서 이렇게 해보는 것을은 어떨까 싶은 것들에 대해 코멘트 남겨봤습니다..!!

YongHoonJJo · 2019-12-19T15:38:05Z

mission001/ganeodolu/js/index.js

+    INVALID_DATA: "data타입이 문자열이 아닙니다.",
+};
+const ENTER_KEY_CODE = 13;
+const ESC_KEY_CODE = 27;


키보드의 키 값들도 위의 error 처럼 한번에 묶어서 관리하는 것은 어떨까요?

객체가 익숙하지 않아서 피해서 사용해왔는데 익숙해지도록 하겠습니다.
e.KeyCode 대신에 e.code로 변경하여 키값들은 삭제하였습니다.

할일에 한글입력시 중복 입력되는 이슈가 있어서 e.code에서 e.key로 변경하였습니다. 그리고 조언대로 역시 묶어서 관리하는게 좋을 것 같아서 수정하였습니다.

mission001/ganeodolu/js/App.js

YongHoonJJo · 2019-12-19T16:04:37Z

mission001/ganeodolu/js/TodoList.js

+        }
+    })
+
+    this.render = function () {


객체가 공통적으로 사용하는 메서드의 경우, prototype 을 통해서 저장하는 것은 어떨까요?

prototype에 대해 이해가 부족하여 너무 늦게 답변을 드렸네요. 말씀하신데로 TodoList.prototype.render를 새로 만들어 수정하였습니다. 그런데 TodoList함수에서 분리하여 prototype을 따로 사용할 때 메모리 효율외에 어떤 장점이 있는지 알려주실 수 있을까요??

mission001/ganeodolu/js/TodoList.js

mission001/ganeodolu/js/App.js

YongHoonJJo · 2019-12-19T16:20:18Z

mission001/ganeodolu/js/TodoList.js

+        this.render()
+    }
+
+    if (this === window) {


new 키워드로 생성자 함수를 만들었는지 체크한 부분을 좋은 것 같습니다. 하지만, 이 예외처리가 다른 생성자 함수들 내부에는 적용되어 있지 않은데, 여기에서만 예외처리를 하신 이유가 따로 있으신가요?

생각없이 기존 코드를 재사용하였는데, 꼭 집어서 말씀해주시니 코드의 의미를 다시 보게 되었습니다. 다른 생성자 함수에도 적용하였습니다.

mission001/ganeodolu/js/TodoInput.js

mission001/ganeodolu/js/App.js

객체화, e.code 변경, 주석삭제

html string 수정, 예외처리 프로토타입제외

mission001/ganeodolu/js/TodoList.js

amorfati0310

수고 많으셨습니다 리뷰가 도움이 되시기를 👍

mission001/ganeodolu/js/App.js

amorfati0310 · 2019-12-26T14:14:39Z

mission001/ganeodolu/js/App.js

+        $target: $todoList,
+        $targetFilter: $todoFilter,
+        data: this.data,
+        onToggleClick: (index) => {


이벤트 핸들러들 이름이 일관성 있어서 좋네요 👍 on prefix
다만 onEditTodo , onChangeTodo on 동사 + 명사 on 붙여진 부분도 더 일관성있게 다루면 어떨까 싶습니다

로토님이 이벤트와 연관된 함수에는 보통 on을 앞에 붙힌다고 사용하고 있었습니다.
명명규칙이 통일되지 않았었네요. on 동사 + 명사 형태로 수정하였습니다.

amorfati0310 · 2019-12-26T14:21:22Z

mission001/ganeodolu/js/TodoCount.js

+        this.render();
+    }
+
+    if (this === window) {


(this === window) instanceOf 를 활용하시면 더 가독성 좋은 코드일 것 같아요
this는 실행 컨텍스트에 따라 달라져서 ex)

// 조금 억지긴 하지만 const app.todoCount = TodoCount app.todoCount({...todoCountdata}) 객체 프로퍼티로 실행되면 this가 app을 가리켜서 일반 function으로 그냥 호출했을때만 this가 전역 객체를 바라봅니다. bind, call, apply 로도 this가 바뀔 수가 있어서 new keyword가 아닌경우 instance로 바로 검출을 하시는게 더 좋을 것 같습니다.

if(!todoList instanceof TodoList || !todoCount instanceof TodoCount){ throw new Error(error.NO_USED_NEW_KEYWORD) }
App 마지막 줄에 instance를 판별하는 구문을 넣어보았습니다. 하지만 new를 사용하지 않으면 인스턴스를 확인하는 줄까지 오기전에 $target이 undefined라는 에러가 나와서 입력한 에러메시지는 나오지 않습니다. 적절한 위치가 있을까요?

amorfati0310 · 2019-12-26T14:22:23Z

mission001/ganeodolu/js/TodoCount.js

+    }
+    this.render();
+
+}


아 궁금해서 이거는 그냥 남겨요
저 빨간 엔터 No new line end of line 저 부분 없애는 방법이 있는 것 같은데 ㅎㅎ 알게되면 공유드리겠습니다
공백이 있어서 그런걸까요?

빨간 엔터가 어떤 부분을 말하는 건지 잘 모르겠어서요. 혹시 다시 설명해주실 수 있나요??

이제야 빨간엔터 의미를 알았네요. 호석님 리뷰를 보니 마지막 줄에 개행을 안해서 그런건가봐요.

amorfati0310 · 2019-12-26T14:31:16Z

mission001/origin/.vscode/settings.json

@@ -0,0 +1,3 @@
+{


.env 랑 dot 도 같이 알아보시면 좋을 것 같습니다 👍

mission001/ganeodolu/js/index.js

amorfati0310 · 2019-12-26T14:33:01Z

mission001/ganeodolu/js/TodoList.js

+        const { className } = e.target;
+        // const { index } = dataset
+        // console.log(e.target)
+        if (className === 'all selected') {


if else 가 많아지면 switch case 혹은 객체로 빼서 대응하는 함수에 맵핑 시키면 가독성이 더 좋아지는데 시도해보시면 좋을 것 같아요 :)

이 부분에서는 다른 로직으로 변경해서 사용하지는 않았지만 다른 부분에서 사용해 보았습니다.
if (className === 'toggle') { onToggleClick(index) } else if (className === 'destroy') { onRemoveClick(index) }
위 코드를 아래코드처럼 수정
switch (className) { case 'toggle': onToggleClick(index) break; case 'destroy': onRemoveClick(index) break; }
다음부터 switch문도 잘 참고하도록 하겠습니다.

amorfati0310 · 2019-12-26T14:34:26Z

mission001/ganeodolu/index.html

+    </ul>
+  </div>
+</section>
+<script src="js/App.js"></script>


이렇게 파일 하나 하나 script를 따로 import 해야 되는 부분을 어떻게 해결할 수 있을지 고민해보시는 것도 좋을 것 같네요 :)

import, export를 활용하여 module로 사용할 index.js만 사용하도록 변경하였습니다.

amorfati0310

다른 분들 리뷰보다가 추가로 보이는 게 있어서 추가 코멘트를 남겨요 도움이 되시기를 바랍니다 ~~

mission001/ganeodolu/js/TodoList.js

eastjun-dev

피드백 요청드린 부분 세심하게 확인해주시고 반영해주셔서 피드백 과정이 즐거웠습니다 :)
호준님만의 속도로 계속해서 정진해나가면서 성장하시길 바랍니다 화이팅! 👏👏👏
저는 approve 진행할게요 :)

Happy 2020 Year !!!

1. if else를 switch로 변환 2. import, export 사용 3. instanceOf 사용하여 instance 검사 프로토타입 미반영

ganeodolu added 3 commits December 16, 2019 16:09

todolist에 todoitem을 엔터키로 추가

3941964

M1-1 todolist 입력 수정

217a5fc

CSS형식에 맞게 todolist의 타겟 변경, innerHTML 태그 추가

M1 2~5 기능구현

2521ce9

2. 토글버튼으로 할일 completed상태변경 3. 삭제버튼으로 할일 삭제 4. 할일 더블클릭시 에디트모드 5. 할일 개수 하단에 표시

eastjun-dev assigned ganeodolu Dec 18, 2019

M1 삭제기능 보완

129e493

data index 위치를 toggle 태그안에서 li 태그로 변경

eastjun-dev self-requested a review December 19, 2019 01:16

eastjun-dev requested changes Dec 19, 2019

View reviewed changes

YongHoonJJo requested changes Dec 19, 2019

View reviewed changes

ganeodolu added 2 commits December 20, 2019 18:41

M1 리뷰 일부반영

b5ed6ce

객체화, e.code 변경, 주석삭제

M1 리뷰 반영

4163679

html string 수정, 예외처리 프로토타입제외

YongHoonJJo reviewed Dec 24, 2019

View reviewed changes

mission001/ganeodolu/js/TodoList.js Outdated Show resolved Hide resolved

M1-6필터기능 구현중

41c18f0

amorfati0310 reviewed Dec 26, 2019

View reviewed changes

mission001/ganeodolu/js/TodoList.js Outdated Show resolved Hide resolved

mission001/ganeodolu/js/TodoList.js Outdated Show resolved Hide resolved

eastjun-dev approved these changes Dec 29, 2019

View reviewed changes

ganeodolu added 3 commits January 1, 2020 00:00

M1-6 필터기능 구현

fe53a46

Happy 2020 Year !!!

M1 리뷰 반영

a3137a4

1. if else를 switch로 변환 2. import, export 사용 3. instanceOf 사용하여 instance 검사 프로토타입 미반영

M1 prototype 사용 및 개행추가

966cec2

[mission001] todolist crud #5

Are you sure you want to change the base?

[mission001] todolist crud #5

Uh oh!

Conversation

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

Uh oh!

Uh oh!

eastjun-dev left a comment

Choose a reason for hiding this comment

1. 필요없는 주석 제거

2. 역할에 따른 함수 분리

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ganeodolu Feb 13, 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amorfati0310 left a comment

Choose a reason for hiding this comment

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ganeodolu commented Dec 18, 2019 •

edited

Loading

ganeodolu Feb 13, 2020 •

edited

Loading