[mission001] todolist crud #2

YongHoonJJo · 2019-12-17T05:30:50Z

#1 Mission001 / TodoList CRUD

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의 상태값을 확인하여, 해야할 일과, 완료한 일을 클릭하면 해당 상태의 아이템만 보여주기
예전에 제가 진행했던 미션을 참고하면서 진행했는데, 구조를 어떤식으로 잡아야 할지, 설계를 하는 것이 역시나 제일 어려웠던 것 같습니다.

…d class 추가)

…시 view 모드로 복귀

eastjun-dev

용훈님 첫번째로 PR 보내주시고, 적극적으로 참여해주셔서 감사합니다 😄
한 메서드에 네이밍된 기능만 있고, 의미가 명확해서 코드를 읽기 좋았습니다~!
세부 코드에 대한건 코드에 코멘트를 작성하였고, 이 코멘트에는 전체적인 부분에 대해서 피드백 드릴게요.

1. 세미콜론 컨벤션

일부 라인은 끝에 세미콜론이 있고, 없는 경우도 있는데요. 통일되게 작성해 주시면 좋을 것 같아요!

2. 파일 분리

지금은 app.js파일 하나에 모든 컴포넌트가 작성되어있는데, 파일별로 분리해서, 한 js파일에 한 컴포넌트를 나타내면 더 좋을 것 같아요!

3. string으로 직접 비교하는 부분

string으로 switch case 같은 경우에서 '직접' 비교하는 경우가 있는데, 해당 부분들을 따로 상수로 관리해 주시면 유지보수와, 협업 측면에서 상태를 추가 및 수정할 때 더 용이할 것 같아요.!

eastjun-dev · 2019-12-18T00:08:04Z

mission001/YongHoonJJo/js/app.js

+function TodoList(selector) {
+  this.items = []
+  this.className = 'all'
+  this.$todoList = document.querySelector(selector) // #todo-list


사용하지 않는 주석은 없애도 괜찮을 것 같아요 :)

eastjun-dev · 2019-12-18T00:09:30Z

mission001/YongHoonJJo/js/app.js

+  this.$filters = document.querySelector('ul.filters')
+
+  this.$filters.addEventListener('click', (e) => {
+    if(e.target === this.$filters) return ;


return ; 에서 return 다음에 공백이 없어도 괜찮을 것 같습니다 :)

개인적으로 세미콜론은 안쓰는 방향으로 하고 있는데, 저부분은 세미콜론이 없으면 어색해서 저렇게 했었네요 ㅜㅜ 통일 시키겠습니다!!

eastjun-dev · 2019-12-18T00:13:16Z

mission001/YongHoonJJo/js/app.js

+    if(e.target === this.$filters) return ;
+
+    const aTags = document.querySelectorAll('ul.filters li a')
+    for(const tag of aTags) tag.classList.remove('selected')


aTags의 모든 Node에서 selected class를 지우는 것보다, selected Class를 포함한 경우에만 remove 하는게 더 좋지 않을까요? 물론 성능상에는 차이가 없을 것 같지만, 의미상 모든 aTags에서 selcted class를 지워야 하는 로직으로 오해할 수 있을 것 같아서요~!

element.classList.contains() 라는 것이 있었네요!! 감사합니다!!

eastjun-dev · 2019-12-18T00:15:39Z

mission001/YongHoonJJo/js/app.js

+
+    const [action, id] = datasetAction.split('-')
+    switch(action) {
+      case 'check': this.toggleState(id); break;


string의 경우 따로 상수로 관리하는게 실수도 줄이고, 상태의 유지보수 측면에서 좋지 않을까요~?
한 곳에서 action의 status를 나타내는 상수가 있다면, 다른 동료가 협업할 때 비슷한 로직 구현 시, 해당 부분에 대한 커뮤니케이션 비용도 줄어들고, 재사용할 수 있을 것 같아요 :)

넵!! 상수 리터럴 부분은 따로 관리해서 진행하는 것이 맞는 것 같습니다. 스터디 하면서 피드백 받았던 것 같은데, 아직 몸에 베지 않은 것 같네요 ㅜ 감사합니다!!

eastjun-dev · 2019-12-18T00:44:18Z

mission001/YongHoonJJo/js/app.js

+  }
+}
+
+TodoList.prototype.activeRender = function(items) {


activeRender 와 completedRender의 경우 renderActive, renderCompleted로 표현하는건 어떨까요? 다른 function 들도 기능을 나타내는 동사 + 목적어나, 상태를 나타내는 형태로 선언하신 것 같은데 2 함수의 경우에는 순서가 반대인 것 같아요. 같은 방식으로 네이밍해주면 더 좋을 것 같습니다 :)

예리하십니다.. ㅎㅎ 적용하겠습니다!!

eastjun-dev · 2019-12-18T01:25:30Z

mission001/YongHoonJJo/js/app.js

+function App(inputSelector, todoListSelector) {
+  this.index = 0
+  this.$input = document.querySelector(inputSelector)
+  this.TodoListComponent = new TodoList(todoListSelector)


변수 네이밍 컨벤션이 todoListComponent가 더 일관될 것 같아요 :)

eastjun-dev · 2019-12-18T01:27:51Z

mission001/YongHoonJJo/js/app.js

+        completed: false,
+      }
+      e.target.value = ''
+      this.TodoListComponent.addState(item)


addState는 상태를 추가한다는 의미인데, 실제 addState 메서드를 보면 todoItem을 리스트에 추가하는 로직이어서 오해가 생길 수 있는 네이밍인 것 같아요. addItem or setState로 하면 어떨까요~?

eastjun-dev · 2019-12-18T01:34:33Z

mission001/YongHoonJJo/js/app.js

+  })
+}
+
+document.addEventListener('DOMContentLoaded', () => {


app.js를 index.html에서 body tag가 끝나기 바로 직전에 호출되기 때문에 DOMContentLoaded eventListner가 없어도 괜찮을 것 같습니다. 다른 개발자들도 이 부분에 대한 궁금증이 있었는지 이미 링크에서도 확인해보실 수 있을 것 같아요 :)

eastjun-dev · 2019-12-18T01:36:07Z

mission001/YongHoonJJo/js/app.js

+}
+
+TodoList.prototype.renderByFilter = function(filter) {
+  switch(filter) {


👍 if else로 길어질 수 있는 부분을 switch case로 깔끔하게 구현해주신 것 같아요 :)

eastjun-dev · 2019-12-18T01:37:19Z

mission001/YongHoonJJo/js/app.js

+  const $liElement = document.querySelector(`li[data-action=edit-${id}]`)
+  const $inputElement = $liElement.querySelector('input.edit')
+
+  $liElement.setAttribute('class', 'editing')


class 추가는 element.classList.add()로 가능하기 때문에 class 다루는 메서드로 활용하는게 더 좋은 것 같아요 :)

저 부분의 코드를 작성할 당시 element.classList.add() 의 존재를 몰랐었습니다...
그리고 다른 부분 코딩할 때 알게되서 적용했는데, 이부분을 이렇게 적용해놨다고 생각을 못했었네요 ㅜ 감사합니다!!

…벤트 제거

StellaKim1230

코드 잘봤습니다.^^

3기 수업할때보다 용훈님 코드 업그레이드 된 느낌이네요 ㅎㅎ

StellaKim1230 · 2019-12-19T13:30:25Z

mission001/YongHoonJJo/js/TodoList.js

+  this.items = []
+  this.filter = ALL
+  this.$todoList = document.querySelector(selector)
+  this.$filters = document.querySelector('ul.filters')


궁금한건데,
document.querySelector('filters') 이렇게는 안가져와져서 document.querySelector('ul.filters') 이렇게 작업하신건가요??

조금 더 구체적으로 표현해보고 싶었습니다..!!

아하 ㅎㅎㅎ 용훈님의 깊은 뜻 알겠습니다^^

StellaKim1230 · 2019-12-19T13:32:03Z

mission001/YongHoonJJo/js/TodoList.js

+    if(!datasetAction) return 
+
+    const [action, id] = datasetAction.split('-')
+    switch(action) {


조건이 1가지 일경우는 if 문 사용하는것이 성능적으로 유리하다고 들었는데,
break 때문에 switch 사용하신건가요??

추후 action 에 대한 종류가 추가될 수 있지 않을까 싶어 switch-case 문으로 작성했던 것 같습니다.!

아아,,ㅎㅎ 깊은 뜻이 있었군요!! ㅎㅎ 답변 감사드립니다.

StellaKim1230 · 2019-12-19T13:33:11Z

mission001/YongHoonJJo/js/TodoList.js

+  this.renderCounterContainer(items)
+}
+
+TodoList.prototype.renderCounterContainer = function(items) {


todoList와 todoCount 는 역할이 달라서 컴포넌트를 분리해줘도 좋을 것 같습니다^^

설계를 어떻게 해야할까 하고 고민하는 과정에서 그부분까지 생각을 못했던 것 같습니다ㅜㅜ todoCount 컴포넌트도 분리 해보겠습니다..!!

저도 늘 설계에서 고민하고 또 고민하죠,,ㅠㅠ 개발자들의 숙명 같습니다 ㅠㅠ

StellaKim1230 · 2019-12-19T13:34:17Z

mission001/YongHoonJJo/js/TodoList.js

+}
+
+TodoList.prototype.removeState = function(id) {
+  this.items = this.items.filter((item) => id != item.id)


저는 개인적으로 item => id != item.id 요런 문법을 선호합니다 ㅎㅎㅎ
생각공유!

YongHoonJJo · 2019-12-19T14:16:14Z

@EastjunDev

소스코드 분리.
세미콜론 컨벤션 통일, 주석 제거
'DOMContentLoaded' 관련 피드백 반영
네이밍 컨벤션 피드백 적용
String literal 상수로 관리
HTML String 하나의 template 으로 처리
element.classList.contains() 활용.

리뷰주셨던 내용들에 대해 모두 반영하였습니다. 꼼꼼히 살펴봐주셔서 감사합니다!!

eastjun-dev · 2019-12-21T05:56:13Z

@YongHoonJJo
용훈님 의견 남겨드린 부분 모두 정확히 반영해주시고, 제 코드에도 좋은 피드백 많이 남겨주셔서 너무 감사드립니다 :) 이번미션은 approve진행할게요 😄

StellaKim1230 · 2019-12-22T04:36:25Z

@YongHoonJJo
컴포넌트 설계에 대해서 같이 고민을 나누면 더 좋을거 같아요 ㅎㅎ (저도 항상 고민중이라...)
이번미션 approve 하겠습니다^^ 👍

amorfati0310

수고 많으셨어요 리뷰가 조금이라도 도움이 되셨기를 ~~ :)

amorfati0310 · 2019-12-28T15:11:35Z

mission001/YongHoonJJo/js/TodoList.js

+  this.$todoList = document.querySelector(selector)
+  this.$filters = document.querySelector('ul.filters')
+
+  this.$filters.addEventListener('click', (e) => {


e.target 이 여러 군데 쓰여서 변수로 저장하면 더 좋을 것 같아요 :)

그렇네요 ㅎㅎ 다음 미션 부터는 프로퍼티 캐싱에 대해서도 적용토록 하겠습니다!! 감샇바니다. ㅎㅎ

amorfati0310 · 2019-12-28T15:12:48Z

mission001/YongHoonJJo/js/TodoList.js

+  this.$filters.addEventListener('click', (e) => {
+    if(e.target === this.$filters) return 
+
+    const aTags = document.querySelectorAll('ul.filters li a')


처음부터 selected 셀렉터로 찾는 건 어떨까요?

와.. 생각 못했었네요..ㅎㅎ 감사합니다!!

amorfati0310 · 2019-12-28T15:14:11Z

mission001/YongHoonJJo/js/TodoList.js

+    if(!datasetAction) return 
+
+    const [action, id] = datasetAction.split('-')
+    switch(action) {


action 들 reducer 같이 쓰는 거 좋네요 :)

amorfati0310 · 2019-12-28T15:17:40Z

mission001/YongHoonJJo/js/TodoList.js

+    const todoItemTemplate = `
+      <li ${completed && 'class="completed"'} data-action=${EDIT}-${id}>
+        <div class="view">
+          <input class="toggle" type="checkbox" data-action=${CHECK}-${id} ${completed && 'checked'}>


data-actinon 과 id 혹은 data-id로 따로 주는 것은 어떨까요?

그렇게 하는 것도 좋은것 같아요!! 제가 아직 경험이 부족해서, 하나에 다 몰아서 넣는 것 밖에 생각을 못했네요 ㅜㅜ

amorfati0310 · 2019-12-28T15:20:39Z

mission001/YongHoonJJo/js/app.js

+  this.$input.addEventListener('keydown', e => {
+    if(e.key === 'Enter') {
+      const item = {
+        id: this.index++,


+=1 를 쓰시는게 더 좋을 것 같아요 :)
https://eslint.org/docs/rules/no-plusplus

@amorfati0310 whitespace 의 차이가 문제를 만들 수 있다는 것은 이해가 되는데요.. 그런한 문제를 만들 가능성을 줄일 수 있기 때문에 추천해주신 방식이 더 좋을 것 같단 의미이신가요??

eslint에서는 그렇게 되어 있네용 :)
js 대가 더글락스 크로포드도 안티패턴으로 소개하고는 있는데 정확한 이유는 잘 모르겠네요 ;ㅁ;
airbnb/javascript#540

YongHoonJJo · 2020-01-06T18:34:18Z

@amorfati0310

e.target 캐싱 처리
document.querySelectorAll('ul.filters li a') 에서 document.querySelectorAll('.selected') 로 변경
data-actinon 과 data-id 분리
++ 연산자 대신, +=1 로 사용.

피드백 주신 내용 모두 반영하였습니다. :)

YongHoonJJo added 8 commits December 14, 2019 21:29

[Init] Copy Default Files

c7b562a

[Mission01] todo list에 todoItem을 키보드로 입력하여 추가

66cf838

[Mission01] todo list의 체크박스를 클릭하여 complete 상태로 변경. (li tag 에 complete…

182d82e

…d class 추가)

[Mission01] todo list의 x버튼을 이용해서 해당 엘리먼트를 삭제

b73dc37

[Mission01] todo list를 더블클릭 했을 때 input 모드로 변경. esc키를 누르면 수정되지 않은 채로 다…

3106328

…시 view 모드로 복귀

[Mission01] todo list의 item갯수를 count한 갯수를 리스트의 하단에 보여주기

79be037

[Mission01] todo list의 상태값을 확인하여, 해야할 일과, 완료한 일을 클릭하면 해당 상태의 아이템만 보여주기

6522617

[Mission01] Fix typo (더블클릭시 input 모드로 변경 후 내용저장)

481aa2b

eastjun-dev assigned YongHoonJJo Dec 18, 2019

eastjun-dev self-requested a review December 18, 2019 00:06

eastjun-dev requested changes Dec 18, 2019

View reviewed changes

YongHoonJJo added 9 commits December 18, 2019 12:39

[Mission01][Feedback] 파일 분리 적용 / live-server 설치

38bc2ee

[Mission01][Feedback] 세미콜론 컨벤션 통일, 주석 제거

b54eea1

[Mission01][Feedback] 클래스 추가 element.classList.add() 로 처리

a2b00c4

[Mission01][Feedback] script 태그 body 최하단에 위치에 따른 'DOMContentLoaded' 이…

026ad54

…벤트 제거

[Mission01][Feedback] 네이밍 컨벤션 피드백 적용

370bdd6

[Mission01][Feedback] string literal 상수로 관리

2bdbdf4

[Mission01][Feedback] html string 하나의 template 변수로 관리

6cb9d84

[Mission01][Feedback] tag.classList.contains() 적용 / filter 관련 코드 리팩토링

bc3a291

[Mission01] item 수정 중 ESC 눌렀을 때, filter 별로 보이도록 수정

bfb7296

YongHoonJJo requested review from StellaKim1230, eastjun-dev and s280493 and removed request for eastjun-dev December 18, 2019 11:49

StellaKim1230 reviewed Dec 19, 2019

View reviewed changes

eastjun-dev approved these changes Dec 21, 2019

View reviewed changes

StellaKim1230 approved these changes Dec 22, 2019

View reviewed changes

amorfati0310 reviewed Dec 28, 2019

View reviewed changes

amorfati0310 approved these changes Jan 3, 2020

View reviewed changes

[Mission01/Feedback] Set Feedback on @amorfati0310

22c2b67

[mission001] todolist crud #2

Are you sure you want to change the base?

[mission001] todolist crud #2

Uh oh!

Conversation

YongHoonJJo commented Dec 17, 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. 파일 분리

3. string으로 직접 비교하는 부분

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

YongHoonJJo commented Dec 19, 2019

Uh oh!

eastjun-dev commented Dec 21, 2019

Uh oh!

StellaKim1230 commented Dec 22, 2019

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!

YongHoonJJo commented Dec 17, 2019 •

edited

Loading