[10기 soonysoo] TodoList with CRUD #211

soonysoo · 2021-07-11T14:31:12Z

안녕하세요 😄
지난 기수에 실력이 부족해서 끝까지 완료하지 못했지만,
이번 기수에는 꼭 3단계 마무리하도록 노력하겠습니다.

🎯 요구사항

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

🎯🎯 심화 요구사항

localStorage에 데이터를 저장하여, TodoItem의 CRUD를 반영하기. 따라서 새로고침하여도 저장된 데이터를 확인할 수 있어야 함

kimjanghu

코드 잘 봤습니다!! 한 주 동안 고생하셨습니다

kimjanghu · 2021-07-12T11:17:32Z

src/components/TodoList.js

+            if(e.key=="Enter"){
+                this.$props.onUpdateTodo(targetDom.id, edit_input.value)
+            }else if(e.key=="Escape"){
+                edit_li.classList.remove('editing')
+            }


if, else if 보다는

if (e.key === "Enter") { # code } if (e.key === 'Escape') { #code }

이런식으로 if 만 사용해서 하면 가독성이 더 좋아질 것 같아요!

kimjanghu · 2021-07-12T11:18:34Z

src/components/TodoList.js

+        const deleteBtns = document.querySelectorAll('.destroy');
+        deleteBtns.forEach(element => {
+            element.addEventListener('click',(e)=>{
+                this.$props.onDeleteTodo(e.target.id);
+            }) 
+        });


각 요소에 이벤트를 각각 걸기보다는, 이벤트 위임을 활용해서 코드 작성하시면 좋을 것 같습니다!

kimjanghu · 2021-07-12T11:19:37Z

src/app.js

+        const todo = JSON.parse(localStorage.getItem("todo"))? 
+        JSON.parse(localStorage.getItem("todo")) : localStorage.setItem("todo", '{"count": 0,"Filtermode" : 0, "List" : []}'); 


setItem, getItem을 유틸로 분리해서 사용한다면 더 깔끔할 것 같습니다!!

kimjanghu · 2021-07-12T11:23:08Z

src/components/Filter.js

+        <span class="todo-count">총 <strong>${totalNum}</strong> 개</span>
+        <ul class="filters">
+          <li>
+            <a class="all ${mode==0?"selected":""}" id="0" href="#">전체보기</a>


이런 코드는 인라인보다는 따로 분리해서 작성하는게 보기 더 좋을 것 같아요!

const type = (mode) => { return mode === 0 ? "selected" : "" }

이런식으로 뺀 다음에 해당 코드에서는 ${type(mode)} 이런식으로 작성해도 좋지 않을까요?

== 동등연산자보다는 === 일치연산자를 사용하시는게 오류 발생시 유지보수 측면에서 좋을 것 같습니다.

kimjanghu · 2021-07-12T11:23:56Z

src/app.js

+import Component from "./core/component.js";
+import Filter from "./components/Filter.js"
+import Input from "./components/Input.js"
+import TodoList from "./components/TodoList.js"


컴포넌트의 분리가 좋네요!

kimjanghu · 2021-07-12T11:25:22Z

src/app.js

+        const id = String(this.$state.count*1+1);
+        const Filtermode = this.$state.Filtermode;
+        const List = [...this.$state.List, {id ,content:content,activate:false}];
+        localStorage.setItem("todo",JSON.stringify({List,count : id*1,Filtermode}));


자주쓰는 변수는 constant로 사용하는 것도 좋을 것 같아요

const TODO = "todo"

kimjanghu · 2021-07-12T11:28:13Z

src/app.js

+          if(todo.id==id){
+                List.push({id:todo.id, content:todo.content, activate:!todo.activate})
+          }else{
+              List.push({id:todo.id,content:todo.content, activate:todo.activate})
+          }


map을 사용하시면 결과값은 array입니다. 이 경우에는 forEach를 사용하여 리스트에 추가하거나

const List = this.$state.List.map(todo => { if(todo.id==id){ return {id:todo.id, content:todo.content, activate:!todo.activate} } return {id:todo.id,content:todo.content, activate:todo.activate} })

이런식으로 작성해도 될 것 같아요! 그리고 if else보다는 if만 사용하면 가독성 측면에서 좋다고 하더라구요

kimjanghu · 2021-07-12T11:32:19Z

src/components/TodoList.js

+        ${todoList.List.map(item =>`
+          <li data-id="${item.id}" class=${item.activate?"completed":"notcompleted"}>
+            <div class="view">
+              <input class="toggle"  id=${item.id} type="checkbox" ${item.activate?"checked":""}/>
+              <label id=${item.id} class="label">${item.content}</label>
+              <button id=${item.id} class="destroy"></button>
+            </div>
+            <input id=${item.id} class="edit" value="${item.content}" />
+          </li>
+          `).join('')}
+         `


분해할당을 적극적으로 사용해보세요!!

todoList.List.map(({ id, activate, content }) => { #code })

이런식으로 작성하면 굳이 item. 을 안붙여도 되기 때문에 가독성 측면에서 좋을 것 같아요

kimjanghu · 2021-07-12T11:33:02Z

src/core/component.js

@@ -0,0 +1,24 @@
+export default class Component {


공통된 컴포넌트의 사용이 좋네요!!

JamieShin0201 · 2021-07-12T11:22:54Z

src/app.js

+        const todo = JSON.parse(localStorage.getItem("todo"))? 
+        JSON.parse(localStorage.getItem("todo")) : localStorage.setItem("todo", '{"count": 0,"Filtermode" : 0, "List" : []}'); 


반복되는 부분을 변수로 만들어도 괜찮을 것 같아요.

JamieShin0201 · 2021-07-12T11:28:33Z

src/components/TodoList.js

+       const edit_li = targetDom.parentNode.parentNode;
+       const edit_input = targetDom.parentNode.nextElementSibling;


혹시 변수명을 스네이크 케이스로 작성하신 이유가 있으신가요?.?

JamieShin0201 · 2021-07-12T11:31:54Z

src/components/TodoList.js

+    template(){
+        const todoList = this.$state;
+        return `
+        ${todoList.List.map(item =>`


{ id, content, activate } 구조분해 할당을 이용해도 좋을 것 같아요!

JamieShin0201 · 2021-07-12T11:39:47Z

src/app.js

+    onAddTodo(content){
+        const id = String(this.$state.count*1+1);
+        const Filtermode = this.$state.Filtermode;
+        const List = [...this.$state.List, {id ,content:content,activate:false}];


{ id, content: content, activate: false} -> { id, content, activate: false } 다음과 같이 content 속성을 단축 할수 있을 것 같아요. (property shorthand)

JamieShin0201 · 2021-07-12T11:40:30Z

src/app.js

+        const id = String(this.$state.count*1+1);
+        const Filtermode = this.$state.Filtermode;
+        const List = [...this.$state.List, {id ,content:content,activate:false}];
+        localStorage.setItem("todo",JSON.stringify({List,count : id*1,Filtermode}));


혹시 id에 1을 곱하는 이유를 알 수 있을까요?

JamieShin0201 · 2021-07-12T11:48:54Z

과제 다 완성하셨군요 :) 한 주간 수고하셨습니다!! ㅎㅎ

ChangSubKwak

더 좋은 리뷰를 해드리고 싶은데 제가 많이 부족해요.
좋은 배움을 줄 수 있는 코드를 보여주셔서 감사합니다.

ChangSubKwak · 2021-07-12T12:06:20Z

src/core/component.js

@@ -0,0 +1,22 @@
+export default class Component {


이렇게 하는 것이 모던한 JS 스타일인 것 같아요. 제가 이전에 본 장후님의 코드도 데이터 관리 부분, 그리는 부분, 컨트롤 하는 부분이 나누어 진 것을 봤어요. 마찬가지로 수은님도 그런 render(그리는 부분), setState(데이터 관리) 부분을 확인할 수 있어서 좋았습니다. 저도 앞으로 이렇게 해야 겠습니다. 😀

ChangSubKwak · 2021-07-12T12:11:15Z

src/app.js

+
+    mounted(){
+        const {onAddTodo, onToggleTodo, onDeleteTodo, onCountTodo, onFilter} = this;
+        const _Input = document.querySelector('#new-todo-title');


변수명에 "_"를 맨앞에 붙이는 것이 임시변수를 뜻하는 것이라고 봐도 된다면, temp를 앞에 쓰는 방법도 있답니다. 예를 들면 tempInput 이건 명시적이라서, 좀더 보기에 편할 수도 있을 것 같은데, 개인취향이고 컨벤션을 따라가는 부분이기에 참고만 하시면 될 것 같아요. ^^

ChangSubKwak · 2021-07-12T12:13:04Z

src/main.js

id에서 class에서 바꾼 의도가 있을 것 같은데요. 궁금해요.

ChangSubKwak · 2021-07-12T12:16:29Z

src/components/Filter.js

+    }
+
+    template(){


혹시, 전체를 템플릿으로 한 부분이 궁금해요. ^^
저렇게 하면, 데이터를 변하는 부분과 고정된 부분을 구별해서 가져오는 것이 쉽지 않을 것 같아서요.
제가 FE를 사용을 잘 몰라서 하는 이야기일 수도 있어요. 그냥 참고만 하셔도 됩니다.

ChangSubKwak · 2021-07-12T12:21:48Z

src/app.js

@@ -67,13 +67,21 @@ class App extends Component{
    }
    onDeleteTodo(id){
        const List = this.$state.List.filter(todo => todo.id!==id);
-        const count = String(this.$state.count*1+1);
+        const count = String(this.$state.count*1);


*1 은 형변환하려고 하실 때 말고 다른 용도로 사용한 것인지 궁금해요. 😀

ChangSubKwak · 2021-07-12T12:29:12Z

src/app.js

+    onUpdateTodo(id, new_content){
+        const List =  [];
+        this.$state.List.map(todo => {
+            if(todo.id==id){


이것도 스타일일 수 있는데요. 보통 if문과 (사이 는 공백 한칸 띄우는 스타일을 요즘 많이 사용하는 분위기 더라고요.
(https://naver.github.io/hackday-conventions-java/#braces-knr-style)

그래서
if(todo.id==id){
를
if (todo.id == id) {
로 쓰는 방법도 있답니다.

soonysoo added 10 commits July 6, 2021 21:33

프로젝트 구성

5a98520

App component 구조화

24ef8da

App구조화

083007d

component 생성

5cb9313

onaddtodo 기능구현

be55af2

state수정

76d96d8

delete activate 기능 구현

023bf9e

Filtering 기능구현

1fb2b3a

updateTodo 기능 구현

e3074f6

요구사항

11807be

kimjanghu reviewed Jul 12, 2021

View reviewed changes

JamieShin0201 reviewed Jul 12, 2021

View reviewed changes

ChangSubKwak reviewed Jul 12, 2021

View reviewed changes

[refactoring] util기능 분류

ab1f0c9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10기 soonysoo] TodoList with CRUD #211

[10기 soonysoo] TodoList with CRUD #211

soonysoo commented Jul 11, 2021

kimjanghu left a comment

kimjanghu Jul 12, 2021

kimjanghu Jul 12, 2021

kimjanghu Jul 12, 2021

kimjanghu Jul 12, 2021

kimjanghu Jul 12, 2021

kimjanghu Jul 12, 2021

kimjanghu Jul 12, 2021

kimjanghu Jul 12, 2021

kimjanghu Jul 12, 2021

JamieShin0201 Jul 12, 2021

JamieShin0201 Jul 12, 2021

JamieShin0201 Jul 12, 2021

JamieShin0201 Jul 12, 2021

JamieShin0201 Jul 12, 2021

JamieShin0201 commented Jul 12, 2021

ChangSubKwak left a comment

ChangSubKwak Jul 12, 2021

ChangSubKwak Jul 12, 2021

ChangSubKwak Jul 12, 2021

ChangSubKwak Jul 12, 2021

ChangSubKwak Jul 12, 2021

ChangSubKwak Jul 12, 2021

		const todo = JSON.parse(localStorage.getItem("todo"))?
		JSON.parse(localStorage.getItem("todo")) : localStorage.setItem("todo", '{"count": 0,"Filtermode" : 0, "List" : []}');

		const edit_li = targetDom.parentNode.parentNode;
		const edit_input = targetDom.parentNode.nextElementSibling;

[10기 soonysoo] TodoList with CRUD #211

Are you sure you want to change the base?

[10기 soonysoo] TodoList with CRUD #211

Conversation

soonysoo commented Jul 11, 2021

🎯 요구사항

🎯🎯 심화 요구사항

kimjanghu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieShin0201 commented Jul 12, 2021

ChangSubKwak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment