[Mission002] TODOS 시스템에 비동기/온오프라인을 고려하여 업데이트하기 #13

YongHoonJJo · 2019-12-25T07:44:52Z

#7 fetch를 이용해 데이터 CRUD 하기

주어진 API 를 통해 CRUD 업데이트
async-await 을 통한 처리 및 그에 따른 try-catch 적용
초기 앱 실행시 Get 을 통해 데이터를 가져오기 때문에, LocalStorage 는 적용하지 않았습니다.

mission01 작업한 곳에서 브랜치를 생성했는데, 기존 미션이 머지가 되지 않아, Mission02 로 된 부분을 보셔야 할 것 같습니다. ㅜㅜ

…d class 추가)

…시 view 모드로 복귀

…벤트 제거

s280493 · 2019-12-29T07:29:45Z

mission002/YongHoonJJo/js/utils/Contants.js

@@ -0,0 +1,16 @@
+export const eventKeyboards = {


상수를 enum화해서 사용하는 게 아주 편리해보입니다!!

s280493 · 2019-12-29T07:29:53Z

mission002/YongHoonJJo/js/utils/apis.js

@@ -0,0 +1,29 @@
+export const getTodoList = async (username) => {
+  return await fetch(`http://todo-api.roto.codes/${username}`)


'http://todo-api.roto.codes/' 는 별도의 상수로 빼서 사용해도 괜찮을 것 같아요 :)

BaseURL 로 따로 뺄가 고민했었는데요.ㅜ 따로 빼도록 하겠습니다!! ㅎㅎ

s280493 · 2019-12-29T07:30:29Z

mission002/YongHoonJJo/js/app.js

+  this.todoListComponent = new TodoList(todoListSelector)
+
+  this.$input.addEventListener('keydown', async e => {
+    if(e.key === 'Enter') {


Constants.js에서 만들어두신 상수를 사용하셔도 좋을 것 같아요.

이전에 피드백 받은 부분 적용하고 있었는데, 완전히 안된 것 같습니다. ㅜ 적용하겠습니다!

s280493

TodoList.js 파일을 조금 전에 읽었는데, TodoList.js 파일이 또 나와서 처음엔 당황했습니다..!
보니까, mission001의 코드가 섞여 있더라구요~
다음에는 PR 날려주실 때, 이전 미션의 코드는 섞이지 않게 날려주시면 더 좋을 것 같아요 :)

s280493 · 2019-12-29T07:33:17Z

mission002/YongHoonJJo/js/TodoList.js

+}
+
+TodoList.prototype.toggleEditView = function(id) {
+  const $liElement = document.querySelector(`li[data-action=edit-${id}]`)


모든 li태그에 data-action=edit이 들어가는 것 같은데,
li 태그를 data-action=edit으로 선택하는 이유가 있는지, 어떤 걸 구분하기 위해 넣은 속성인지 궁금합니다.
단순히 li태그끼리 구분하기 위해서라면, id로 구분해도 될 것 같아서요!

지금보니 그렇네요!! data-* 관련 attribute 를 사용하는게 익숙하지 않아서, 그랬던게 아닌가 싶습니다. ㅜㅜ 감사합니다!!

s280493 · 2019-12-29T07:33:47Z

mission002/YongHoonJJo/js/TodoList.js

+  const itemsHtmlString = items.reduce((acc, item) => {
+    const { _id, content, isCompleted } = item
+    const todoItemTemplate = `
+      <li ${isCompleted && 'class="completed"'} data-action=${EDIT}-${_id}>


completed가 false일 때, li 태그에 false라는 attribute가 적용됩니다.
{ completed ? class="completed" : "" }로 바꾸면 없어질거에요.

더 짧게 쓸 생각만 햇었네요 ㅜㅜ 적용하겠습니다!!

s280493 · 2019-12-29T07:34:28Z

mission002/YongHoonJJo/js/TodoList.js

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


바로 위의 이슈와 같은 이슈입니다.
{ completed ? "checked" : "" } 로 바꾸는 건 어떨지 제안드립니다

eastjun-dev

용훈님 코드보면서 제가 생각치 못한 구현 방식을 발견하곤 합니다 ㅎㅎ 덕분에 피드백 남기면서 저도 배워갈 수 있는 접근 방식이 많은 것 같아요 :)

몇가지 피드백 남겼는데 리팩토링 작업하실 때 mission002인 경우 mission001에 dependecy가 걸리지 않게, mission002 directory를 따로 만들어서 작업을 진행해주셔야 이전 미션의 merge와 상관없이 원활하게 진행하실 수 있을 것 같아요 :)

eastjun-dev · 2019-12-29T17:54:15Z

mission002/YongHoonJJo/js/TodoList.js

+  })
+
+  this.$todoList.addEventListener('click', async (e) => {
+    const { CHECK, REMOVE } = dataActions


addEventListener에 바인딩하는 구현부분을 따로 함수로 빼면 어떨까요?
지금 같은 경우는 $todoList를 click했을 때 어떤 처리를 해주는지 구현부분을 모두 읽어봐야 확인할 수 있는데 만약 this.$todoList.addEventListener('click', eventDispatch)와 같은 형태로 처리해주면 구현 부분을 모두 읽지 않아도
클릭이벤트에 해당하는 내용을 보다 쉽게 알 수 있을 것 같아요 :)

eventHandler 모두 별도로 분리하였습니다!

eastjun-dev · 2019-12-29T17:59:05Z

mission002/YongHoonJJo/js/TodoList.js

+}
+
+TodoList.prototype.renderByFilter = async function() {
+  const res = await getTodoList(this.username)


res 변수를 재사용하지 않는다면, fetch하는 부분에서
fetch(uri, config).then((response) => response.json())
위와 같이 변경하면 보다 좋을 것 같아요~!

바로 다음라인에서 사용합니다!
그리고 리뷰주신 내용은, this.item = await fetch(uri, config).then(res => res.json()) 와 같이 사용하란 말씀이신가요??

eastjun-dev · 2019-12-29T18:04:08Z

mission001/YongHoonJJo/js/TodoList.js

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


line 89에서는 countContainer라고 되어있는데, Counter or Count로 통일시키면 더 일관될 것 같아요~!
그리고 실제 render하는 것은 counterContainer가 아니라, count text이니 함수명을 변경해보는건 어떨까요 ? :)

환결성정 다음으로 네이밍이 세상에서 제일 어려운 것 같습니다.ㅜㅜ

저도 너무 어렵네요 ㅠㅠ 네이밍 잘하게 되면 같이 작명소를 지어봐요 ㅠㅠ

eastjun-dev · 2019-12-29T18:05:17Z

mission001/YongHoonJJo/js/TodoList.js

+}
+
+TodoList.prototype.toggleState = function(id) {
+  this.items = this.items.map((item) => id == item.id ? ({...item, completed: !item.completed}) : ({...item}))


혹시 id 비교를 ===가 아닌 ==로 하신 이유가 있으실까요~? 👀

item.id 는 number 이고, id 는 string 이라 일부러 == 로 했습니다. 음.. 나쁜 방법일까요? ㅜㅜ

eastjun-dev · 2019-12-29T18:09:55Z

mission002/YongHoonJJo/js/TodoList.js

+  await this.renderByFilter()
+}
+
+TodoList.prototype.removeState = async function(id) {


toggleState의 경우 상태를 toggle한다는 의미가 적절한 것 같은데
removeState는 상태를 remove한다는 것으로 오해할 수 있는 여지가 있는 것 같습니다 ㅠㅠ

네이밍 추천 부탁드립니다...ㅜㅜ

eastjun-dev · 2019-12-29T18:20:05Z

mission002/YongHoonJJo/js/app.js

+import TodoList from './TodoList.js'
+
+function App(inputSelector, todoListSelector) {
+  this.index = 0


사용하지 않는 변수라면 없어도 괜찮을 것 같습니다 :)

api 적용하면서 뺏어야 했는데, 못본 것 같습니다!! 감사합니다!!

eastjun-dev · 2019-12-30T02:58:58Z

mission001/YongHoonJJo/js/TodoList.js

+}
+
+TodoList.prototype.toggleEditView = function(id) {
+  const $liElement = document.querySelector(`li[data-action=edit-${id}]`)


$표시로 dom Element인 것을 나타내므로 변수명에 *Element는 없어도 괜찮지 않을까요~?

$ 는 로토가 제안해준 방식이다보니 적용한건데, 제가 아직 적응되지 않았던 모양입니다. ㅎㅎ 적용하겠습니다!!

ganeodolu · 2020-01-06T09:09:54Z

mission001/YongHoonJJo/js/TodoList.js

+  this.$todoList.addEventListener('dblclick', (e) => {
+    const { EDIT } = dataActions
+    const datasetAction = e.target.dataset.action
+    if(!datasetAction) return 
+
+    const [action, id] = datasetAction.split('-')
+    switch(action) {
+      case EDIT: this.toggleEditView(id); break
+    } 
+  })
+}


api를 이용할 때에도 에디트 기능을 추가하는 것이 아니시면 삭제해도 되지 않을까 생각합니다.

제가 이해가 잘 안되서 그러는데요ㅜ 좀더 자세하게 설명해주실 수 있을까요?

제가 미션1 파일인 줄 모르고 이곳에 리뷰를 남겨서 혼동을 일으켜 드렸네요.
할일목록을 더블클릭하였을 때, 할일 내용이 POST 메소드를 이용하여 수정되는 것이 아니라면 수정기능을 삭제 해도 되지 않을까 싶어서 코멘트를 남겨보았었습니다.

YongHoonJJo · 2020-01-07T04:37:53Z

@s280493

BaseURL 분리
Constants.js에서 만들어둔 상수 활용
data-action=edit 제거. id 로 구분하기
${isCompleted && 'class="completed"'} 삼항연산자로 변경 처리
{ completed ? "checked" : "" } 삼항연산자로 변경 처리

제가 브랜치 생성하는 과정을 착각해서, 미션2를 이렇게 진행한 것 같습니다. 다음부터는 브랜치 잘 생성해서 진행하겠습니다 .ㅜㅜ 리뷰 감사합니다!!

YongHoonJJo · 2020-01-07T04:43:53Z

@EastjunDev

eventHandler 분리
Counter 대신 Count 로 네이밍 변경하였습니다.
removeState 대신 removeItem 으로 네이밍 변경
this.index = 0 사용하지 않는 변수 제거
컴포넌트 네이밍 관련, $표시로 인한 *Element 제거

mission1 머지 되면, mission2 에도 mission1 에서 변경된 사항 적용한다음 머지시키면 문제 없을 것 같습니다.
제가 브랜치를 잘못생성해서 번거롭게 되었네요 ㅜㅜ

res 변수를 재사용하지 않는다면, fetch하는 부분에서
fetch(uri, config).then((response) => response.json())
위와 같이 변경하면 보다 좋을 것 같아요~!

라고 해주신 부분은 이해가 잘 안되서, 답글 남겼는데, 아직 답변이 없으셔서 이 내용에 대해서는 적용하지 못하였습니다.ㅜㅜ

===가 아닌 ==로 하신 이유에 대한 문의도, 답글 남겼는데, 아직 답변이 없으셔서 적용하지 못했습니다!!

리뷰 감사합니다!!!!

YongHoonJJo added 21 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

[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

[Mission02][init] mission01 내용 copy

23f018b

[Mission02][Add] APIs 추가

79ba9e0

[Mission02][Update] Api 를 통한 데이터 처리

eaf7b2b

[Mission02][Update] try-catch 문 적용

aa14b33

YongHoonJJo requested review from eastjun-dev, ganeodolu and s280493 December 25, 2019 07:45

YongHoonJJo self-assigned this Dec 25, 2019

YongHoonJJo changed the title ~~[Mission02] fetch를 이용해 데이터 CRUD 하기~~ [Mission002] fetch를 이용해 데이터 CRUD 하기 Dec 25, 2019

YongHoonJJo changed the title ~~[Mission002] fetch를 이용해 데이터 CRUD 하기~~ [Mission002] TODOS 시스템에 비동기/온오프라인을 고려하여 업데이트하기 Dec 26, 2019

s280493 reviewed Dec 29, 2019

View reviewed changes

s280493 requested changes Dec 29, 2019

View reviewed changes

eastjun-dev requested changes Dec 30, 2019

View reviewed changes

ganeodolu reviewed Jan 6, 2020

View reviewed changes

YongHoonJJo added 2 commits January 7, 2020 04:12

[Mission2/Feedback] Set Feedback on @s280493

b0b8bc5

[Mission2/Feedback] Set Feedback on @EastjunDev

64f6307

		@@ -0,0 +1,29 @@
		export const getTodoList = async (username) => {
		return await fetch(`http://todo-api.roto.codes/${username}`)

[Mission002] TODOS 시스템에 비동기/온오프라인을 고려하여 업데이트하기 #13

Are you sure you want to change the base?

[Mission002] TODOS 시스템에 비동기/온오프라인을 고려하여 업데이트하기 #13

Uh oh!

Conversation

YongHoonJJo commented Dec 25, 2019 • edited 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!

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

eastjun-dev left a comment • edited by ganeodolu 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!

YongHoonJJo Dec 31, 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!

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!

YongHoonJJo commented Jan 7, 2020

Uh oh!

YongHoonJJo commented Jan 7, 2020

Uh oh!

Reviewers

YongHoonJJo commented Dec 25, 2019 •

edited

Loading

eastjun-dev left a comment •

edited by ganeodolu

Loading

YongHoonJJo Dec 31, 2019 •

edited

Loading