[mission001] todolist crud by HoseokNa · Pull Request #34 · eastjun-dev/frontend

HoseokNa · 2020-01-21T07:25:36Z

미션내용

진행상황

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

소감

스터디에 진행했던 내용이라 금방 마무리 할 줄 알았는데 오래 걸렸네요 ㅠㅠ
따로 리뷰어가 지정 안되어 있어서 우선 PR 올리겠습니다.

가장 어려웠던 점은 list의 상태값에 때라 분류하는 기능이였습니다.
분류하는거 자체는 구현할 수 있었지만 분류 후에도 같은 데이터를 유지하도록 구현하는게 어려웠습니다.
App 컴포넌트에서 전체의 data를 관리하고 todoList render할 때는 data 중에 active와 completed에 맞게 filter 후에 render 해줬습니다.
문제는 인덱스를 li 순서대로 정해 놓았기 때문에 todo를 갱신할 때 정상 동작을 못하고 있네요. 고유 id로 구분하면 해결 될 것 같은데 해결법이 딱히 떠오르지 않아 현재 상태로 우선 제출합니다.

그리고 validation 부분도 잘 안됐습니다. target 엘리먼트가 있는지와 타이핑시 공백으로 입력을 주는지 2가지만 확인하고 있습니다.

전체적으로 로토가 알려주셨던 구조를 유지하려고 했습니다.
각 컴포넌트는 각자의 할 일을 수행하고 App에서 data와 component를 사용하도록.
하지만 미션을 수행 할 수록 난잡해진 부분이 있네요.
더 열심히 해야겠습니다.

Copy origin files to this branch.

Todolist is toggled when it is clicked. it means "completed todo"

When 'X' button is clicked, it is removed

User can modify todos when it was double clicked.

Use toggleTodo function.

Use removeTodo function.

Todos is rendered by filter

Modify rendering about "selected" class name.

Use name convention "map".

Get only count not todo data.

ganeodolu · 2020-01-21T10:55:28Z

mission001/hsna7024/js/TodoInput.js

+  }
+
+  $target.addEventListener("keydown", e => {
+    if (e.keyCode === keyCodeMap.ENTER && $target.value) {


https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode
이 스터디에서 자주 나온 얘기인데 e.keyCode가 위 링크처럼 웹표준에서 제거되어 더 이상 추천하지 않는다고 합니다. 그래서 e.code나 e.key를 사용할 수 있는데 저는 e.key를 추천드립니다.
사용법은 e.key === "Enter" 이런 식으로 사용하시면 됩니다. 현재처럼 constant를 사용하시면 더욱 좋구요. e.keyCode를 사용한 다른 부분도 수정하시면 좋을 것 같아요.

친절한 설명 감사합니다. keyCode 대신에 key를 사용하도록 수정했습니다

eastjun-dev

호석님 제가 리뷰를 너무 늦게 달아드려서 죄송합니다 ㅠㅠ
호석님 구조를 너무 잘짜셔서 보면서 저도 제 옛날 코드들 다시 반성하게 되는 계기가 되었어요 ㅎㅎ
몇가지 사소한 부분들만 더 신경써주시면 훨씬 더 좋은 코드가 될 것 같은데요. 3가지만 더 봐주시면 좋을 것 같아요!

컨벤션
공백 컨벤션이나 띄어쓰기, 인덴트가 안맞는 경우들이 있습니다. eslint 적용하시면 한순간에 해결 하실 수 있을 것 같아요 :)
deprecated된 메서드
toElement같은 deprecated된 메서드를 사용하시는게 눈에 띄는데요. mdn문서들을 참고하셔서 업데이트 해주시면 좋을 것 같아요 :)
if else
저도 많이 하던 실수인데, if문만을 반복해서 하다보면 if문 가장 마지막에 조건이 걸리는 이벤트의 경우 앞의 모든 if문 비교가 되야하는 비용이 추가로 발생을 합니다. if else if로 필요한 조건검사를 하고 나면 바로 종료될 수 있게 변경해주시면 좋을 것 같아요!

첫미션부터 열심히 참여해주셔서 진심으로 감사합니다 😄

eastjun-dev · 2020-02-09T03:16:51Z

mission001/hsna7024/js/TodoList.js

+  }
+
+  $target.addEventListener("click", e => {
+    const { id } = e.toElement.parentElement.parentElement.dataset;


parentElement로 계속 찾기 보다는 closest를 사용하면 어떨까요? 만약 마크업 구조에 다른 tag가 추가된다면 해당 부분이 에러를 발생시킬수도 있을 것 같아요 :)

toElement 사용하는걸 처음봐서 찾아보니 잘 사용되지 않는 deprecated 문법인 것 같아요~
fromElement and toElement are IE non-standard ways to obtain the relatedTarget.
stackoverFlow 링크를 참조하시면 좋을 것 같습니다 :)

넵 closest 사용하도록 수정했습니다. 감사합니다!

eastjun-dev · 2020-02-09T03:20:41Z

mission001/hsna7024/js/TodoList.js

+
+  $target.addEventListener("click", e => {
+    const { id } = e.toElement.parentElement.parentElement.dataset;
+    if (e.target.className === classNameMap.TOGGLE) {


마크업상 새로운 class가 target대상에 추가된다면, 이 부분이 에러가 발생할 수 있을 것 같아요.
classList의 includes 메서드를 활용하시면 좋을 것 같아요 :)

넵 classList 사용하도록 수정했습니다. 감사합니다!

eastjun-dev · 2020-02-09T03:21:16Z

mission001/hsna7024/js/TodoList.js

+    if (e.target.className === classNameMap.TOGGLE) {
+      toggleTodo(id);
+    }
+    if (e.target.className === classNameMap.REMOVE) {


현재 if else가 아니고 if문의 연속이어서 이벤트가 추가 될수록, 모든 if문 로직을 거쳐야 하는 비용이 발생하는 것 같아요.
if else if로 만족한 조건을 찾으면 뒷부분의 로직을 거치지 않게끔 하는게 더 좋을 것 같아요 :)

세심한 리뷰 감사합니다. else if 로 수정했습니다.

eastjun-dev · 2020-02-09T03:21:29Z

mission001/hsna7024/js/TodoList.js

+
+  $target.addEventListener("dblclick", e => {
+    if (e.target.className === classNameMap.LABEL) {
+      const { id } = e.toElement.parentElement.parentElement.dataset;


parentElement로 계속 찾기 보다는 closest를 사용하면 어떨까요? 만약 마크업 구조에 다른 tag가 추가된다면 해당 부분이 에러를 발생시킬수도 있을 것 같아요 :)

마찬가지로 closest 사용해서 적용했습니다.

eastjun-dev · 2020-02-09T03:21:51Z

mission001/hsna7024/js/TodoList.js

+  });
+
+  $target.addEventListener("dblclick", e => {
+    if (e.target.className === classNameMap.LABEL) {


마크업상 새로운 class가 target대상에 추가된다면, 이 부분이 에러가 발생할 수 있을 것 같아요.
classList의 includes 메서드를 활용하시면 좋을 것 같아요 :)

classList 사용하도록 수정했습니다. 감사합니다. 위에 공유해주신 mdn 문서에 contains라는 매서드가 있어서 그걸 사용했습니다.

eastjun-dev · 2020-02-09T03:25:47Z

mission001/hsna7024/js/utils/templates.js

+import { filterMap } from "./constants.js";
+
+export const todoListTemplate = (todo, index) => {
+  const contentHtmlString = `<div class="view"> 


html 구조를 확인할 수 있게 들여쓰기를 맞춰주시면 협업하는 다른 사람들도 같이 확인하기 좋을 것 같아요!

const contentHtmlString = `<div class="view"> <input class="toggle" type="checkbox" ${todo.isCompleted ? "checked" : ""}> <label class="label">${todo.content}</label> <button class="destroy"></button> </div> <input class="edit" value="${todo.content}">`;

넵. 들여쓰기 맞추어 수정했습니다. 감사합니다.

eastjun-dev · 2020-02-09T03:27:38Z

mission001/hsna7024/js/utils/templates.js

+  const activeSelected = filter === filterMap.ACTIVE ? " selected" : "";
+  const completedSelected = filter === filterMap.COMPLETED ? " selected" : "";
+
+  return `<li>


들여쓰기 컨벤션을 같이 맞춰주시면 더 좋을 것 같아요 :)

넵 피드백 반영했습니다.

eastjun-dev · 2020-02-09T03:47:55Z

mission001/hsna7024/js/TodoList.js

+  $target.addEventListener("keydown", e => {
+    if (e.target.className === classNameMap.EDIT) {
+      const { id } = e.target.parentElement.dataset;
+      if (e.key === keyMap.ENTER && e.target.value) {


현재 if else가 아니고 if문의 연속이어서 키보드 이벤트가 추가 될수록, 모든 if문 로직을 거쳐야 하는 비용이 발생하는 것 같아요.
if else if로 만족한 조건을 찾으면 뒷부분의 로직을 거치지 않게끔 하는게 더 좋을 것 같아요 :)

넵. 피드백 반영했습니다. 감사합니다.

eastjun-dev · 2020-02-09T03:49:50Z

mission001/hsna7024/js/TodoInput.js

+  }
+
+  $target.addEventListener("keydown", e => {
+    if (e.key === keyMap.ENTER  && $target.value) {


6line의 경우에는 if 다음에 공백이 없는 반면, 이 11line은 if다음에 공백이 있는데 한 가지 컨벤션으로 통일시켜주시면 좋을 것 같습니다 :) 이참에 eslint를 적용하시는 것도 좋을 것 같아요!

그리고 &&앞에 공백이 2칸인데 1칸으로 줄여주시면 좋을 것 같아요~!

eslint 적용해서 코드 전체적으로 다듬어봤습니다. 세심한 피드백 감사합니다.

eastjun-dev · 2020-02-09T03:54:37Z

mission001/hsna7024/js/TodoCount.js

+  let count = params.count || 0;
+
+  if($target === null) {
+    throw new Error(errorMessageMap.IS_NO_TARGET);


constants.js를 import하지 않는 상태에서 errorMessageMap을 어떻게 사용하고 계신건지 궁금합니다 ㅎㅎ

여기 말고도 import를 안한 컴포넌트가 있었네요. 놓쳤습니다 ㅠㅠ import 안한 상태에서는 동작 안합니다. import해서 수정했습니다. 꼼꼼한 리뷰 감사합니다.

ganeodolu

코드가 보기에도 깔끔하고, 기초가 좋으신 것 같아요. init 함수와 params로 묶어서 관리하는 방식이 좋아보여서 나중에 저도 시도해봐야겠어요. approve하겠습니다. 😆

eastjun-dev

피드백 드린 부분 꼼꼼히 살펴주시고 반영해주셔서 감사합니다~
같이 점점 더 깔끔하고 좋은 코드 만들어 나가요~ 😄

HoseokNa added 23 commits January 20, 2020 14:22

feat: Init mission1

6d86425

Copy origin files to this branch.

feat: Create 'TodoList' component

0e6d732

feat: Create 'App' component

654233f

feat: Create 'TodoInput' component

db44dc6

refactor: Use destructing in todolist

54ce9d6

feat: Add toggling in todolist

5c52283

Todolist is toggled when it is clicked. it means "completed todo"

feat: Add remove todos

1399626

When 'X' button is clicked, it is removed

feat: Add editing todo existed

ac8662c

User can modify todos when it was double clicked.

feat: Create 'TodoCount' component

957e3e9

refactor: Modfiy toggling todo

83917e2

Use toggleTodo function.

refactor: Modfiy removing todo

bbde6db

Use removeTodo function.

feat: Create 'TodoFilter' component

69f590e

Todos is rendered by filter

refactor: Use constants for class name

f3e29a1

refactor: Use constatns key codes

9deab84

refactor: Use html templates

c2e99a9

refactor: Use constants

eec0051

fix: Fix rendering todoFilter

e8985b6

refactor: Modify filterTodo

ef1e2a1

Modify rendering about "selected" class name.

refactor: Modfiy constants name

f2014d0

Use name convention "map".

refactor: Reduce duplicated code

370cc7c

refactor: Check target element

b51304c

refactor: Check empty value in typing

0672dfe

refactor: Modfiy todoCount

15703f0

Get only count not todo data.

ganeodolu reviewed Jan 21, 2020

View reviewed changes

ganeodolu assigned HoseokNa Jan 21, 2020

refactor: Use key instead of keycode

1b2fda3

eastjun-dev requested review from amorfati0310, eastjun-dev and ganeodolu January 24, 2020 01:40

eastjun-dev requested changes Feb 9, 2020

View reviewed changes

HoseokNa added 7 commits February 9, 2020 22:53

refactor: Use closest

5b871bb

refactor: Use else if

a5ea933

refactor: Use classList.contains

51dd4f4

refactor: Use classList.contains

08cb4db

refactor: Modify indent in template

05abded

refactor: Use eslint

24e0f33

docs: Create .gitignore

850d85e

ganeodolu approved these changes Feb 15, 2020

View reviewed changes

eastjun-dev approved these changes Feb 16, 2020

View reviewed changes

Conversation

HoseokNa commented Jan 21, 2020

미션내용

진행상황

소감

Uh oh!

ganeodolu Jan 21, 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!

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!

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!

ganeodolu left a comment

Choose a reason for hiding this comment

Uh oh!

eastjun-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ganeodolu Jan 21, 2020 •

edited

Loading