[2주차] 함초롬 미션 제출합니다. by chorom-ham · Pull Request #3 · CEOS-Developers/react-todo

chorom-ham · 2019-10-06T07:36:40Z

계속 오류가 나서 힘들었던 미션이었습니다 😢
arrow function 문법을 몰라 바인딩에 문제가 있었습니다. arrow function 문법을 다시 한 번 확인하겠습니다.
빈 배열 선언할 때 '=[]'로 하지 않아서 오류가 났는데 앞으로는 이런 실수했던 경험 잘 기억하고 같은 실수 없도록 하겠습니다!

감사합니다. 🙇‍♀

greatSumini

ㅋㅋㅋ 오늘의 삽질은 최신문법의 소중함을 깨닫는 좋은 기회라고 생각해주셨으면 좋겠습니다.
고생하셨습니다! 리뷰 반영 후 커밋해주세요~

greatSumini · 2019-10-06T11:36:10Z

README.md

@@ -1,32 +1 @@
-# react-todo


README.md도 작성해주세요 😂

greatSumini · 2019-10-06T11:36:58Z

src/components/todo/Todo.js

 import TodoInput from './TodoInput';
 import TodoCard from './TodoCard';

+const Div = styled.div`


이 Component의 의도와 역할을 더 잘 설명할 수 잇는 네이밍이 좋을 것 같아요.
ex) Wrapper, Container, ...

네이밍 조언 감사합니다:)

greatSumini · 2019-10-06T11:38:12Z

src/components/todo/Todo.js

+    if (this.state.todoList.length === 0) {
+      return "표시할 TODO가 없어요!";
+    }
+    else {


위의 if문에서 true로 걸렸다면 이미 return 됐을테니까, 여기서 else라고 써줄 필요가 없습니다!

앗 그렇네요..!

greatSumini · 2019-10-06T11:39:01Z

src/components/todo/Todo.js

+
+  getTodo = () => {
+    if (this.state.todoList.length === 0) {
+      return "표시할 TODO가 없어요!";


오늘 배운 조건부 렌더링을 활용해 리팩토링 해보는건 어떨까요? 참고할만한 글의 논리 && 연산자가 있는 인라인 조건을 참고해주세요!

넵 리팩토링해보겠습니다

리팩토링 시도해봤는데 리턴에 계속 밑줄이 그어지면서 오류라고 뜨네요ㅜㅜ

앗 완전히 구조를 바꿔야해요! ㅎㅎ.. 작성스타일 차이로 볼 수도 있는 부분이라고 생각해서 꼭 고치진 않으셔도 됩니당

greatSumini · 2019-10-06T11:39:35Z

src/components/todo/Todo.js

+    }
+    else {
+      let showTodoList = [];
+      for (let index = 0; index < this.state.todoList.length; index++) {


map함수를 사용해 리팩토링 해봐도 좋을 것 같아욯ㅎ

map함수 syntax를 익혀보도록 하겠습니다! 아직 안 익숙하다보니 계속 익숙한 방식만 쓰게되네요 😅

greatSumini · 2019-10-06T11:40:15Z

src/components/todo/Todo.js

+    let newTodoList = Array.from(this.state.todoList);
+    newTodoList.push(_todo);
+    this.setState({
+      todoList: newTodoList


Suggested change

todoList: newTodoList

todoList: [...this.state.todoList, _todo]

spread operator를 이용해 깔끔하게 리팩토링할 수 있습니다~

훨씬 깔끔한 느낌이네요! 감사합니다

greatSumini · 2019-10-06T11:40:43Z

src/components/todo/TodoCard.js

+import React, { Component } from 'react';
 import styled from 'styled-components';

+const Div = styled.div`


이것 역시 더 좋은 네이밍을 찾을 수 있을 것 같아요! div는 아무것도 나타내지 못하거든요 :)

넵 네이밍 더 신경쓰도록 하겠습니다.

greatSumini · 2019-10-06T11:41:25Z

src/components/todo/TodoInput.js

  render() {
-    return <div>화이팅^^</div>;
+    return (
+      <form name="addListForm"


기본 제공되는 tag를 사용하지 않도록 리팩토링 해보세요!

넵 기본 제공 태그를 지양하도록 하겠습니다

greatSumini · 2019-10-06T11:42:09Z

src/components/todo/Todo.js

  }

+  onClick = (index) => {
+    console.log(this.state.todoList)


제가 추가한 코드네요 😂
이런 테스팅용 코드는 PR시에 모두 제거해주셔야합니다!

다 지웠다고 생각했는데 남아있었군요..! 확인했습니다

…o chorom

greatSumini

고생하셨습니다! 추가 리뷰 해드렸으니 참고해주세요~

greatSumini · 2019-10-07T08:50:56Z

src/components/todo/Todo.js

-      return showTodoList;
-    }
+
+    let showTodoList = this.state.todoList.map((_todo, index) => {


추가적인 수정사항이 없으므로 let보다는 const가 적합해보이네요

map함수의 결과를 그대로 return 해주셔도 됩니당

넵 수정하겠습니다

greatSumini · 2019-10-07T08:51:10Z

src/components/todo/Todo.js

-    newTodoList.push(_todo);
    this.setState({
-      todoList: newTodoList
+      todoList: [...this.state.todoList,_todo]


greatSumini · 2019-10-07T08:53:06Z

src/components/todo/Todo.js

-    console.log(this.state.todoList)
    let newTodoList = Array.from(this.state.todoList)
    newTodoList.splice(index, 1);
    this.setState({ todoList: newTodoList });


이 부분도 spread operator와 slice를 이용해서 리팩토링 해보시겠어요?

넵 해보겠습니다!

…o chorom

chorom-ham · 2019-10-07T15:40:43Z

수정 완료했습니다~!

greatSumini

고생하셨습니다 ㅎㅎ 점점 코드 질이 좋아지는게 눈에 보여서 제가 다 뿌듯하네요 👍
리뷰 반영 후 커밋부탁드립니다~

greatSumini · 2019-10-08T12:09:12Z

src/components/todo/TodoInput.js

-    return <div>화이팅^^</div>;
+    return (
+      <Form name="addListForm"
+        onSubmit={(e) => {


함수를 분리해서 써볼까요~

greatSumini · 2019-10-08T12:11:15Z

src/components/todo/TodoInput.js

+        }
+        }
+      >
+        <Row>


Row를 없애고 Row의 스타일링을 Form에 넣어줘도될 것 같아요~

greatSumini · 2019-10-08T12:12:06Z

src/components/todo/Todo.js

 import TodoInput from './TodoInput';
 import TodoCard from './TodoCard';

+const Wrapper = styled.div`


Component 작성시 중요한 정보 순서대로 위쪽에 배치하는게 좋습니다!
컴포넌트들의 스타일링들은 비교적 중요도가 떨어지겠죠?!
맨 아래에 위치시켜주시면 됩니다~

greatSumini · 2019-10-08T12:14:22Z

src/components/todo/Todo.js

    };
  }

+  onClick = (index) => {


Todo 컴포넌트 안에 onClick 함수가 있으니 마치 Todo 컴포넌트를 클릭했을때 발동할 것 같다는 오해의 소지가 있네요~
다른 네이밍으로 바꾸는게 좋을 것 같아요!

greatSumini · 2019-10-08T12:19:59Z

src/components/todo/Todo.js

  }

+  onClick = (index) => {
+    this.setState({ todoList: [...this.state.todoList.slice(0,index),...this.state.todoList.slice(index+1)] });


와우 너무 잘 해주셨어요!!

chorom-ham · 2019-10-08T14:22:13Z

피드백 반영해서 수정했습니다. 피드백 덕분에 더 좋은 코드에 대한 감도 생기고 더 나은 코드를 짤 수 있는 것 같습니다! 감사합니다.

chorom-ham and others added 6 commits October 6, 2019 00:04

TodoInput 수정

b3a3b9c

TodoInput 약간 수정, Todocard 및 Todo 코드 추가

8693d70

todo 수정. index값 증가 안되는 오류 있음.

ce1b9f8

style 추가.. index값 증가 안되는 오류 그대로.

17c5fd0

오류 수정

9a94235

README 삭제

73e11f9

greatSumini changed the base branch from master to chorom-ham October 6, 2019 11:33

greatSumini requested changes Oct 6, 2019

View reviewed changes

chorom-ham added 2 commits October 7, 2019 17:40

피드백 사항 수정

40d76a7

Merge branch 'chorom' of https://github.com/chorom-ham/react-todo int…

512309f

…o chorom

greatSumini requested changes Oct 7, 2019

View reviewed changes

chorom-ham and others added 3 commits October 7, 2019 18:17

Update README.md

aa17100

2차 피드백 반영 수정

163f3c8

Merge branch 'chorom' of https://github.com/chorom-ham/react-todo int…

6732b8c

…o chorom

greatSumini requested changes Oct 8, 2019

View reviewed changes

greatSumini reviewed Oct 8, 2019

View reviewed changes

3차 피드백 반영 수정

d661874

	todoList: newTodoList
	todoList: [...this.state.todoList, _todo]

Conversation

chorom-ham commented Oct 6, 2019

Uh oh!

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

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

chorom-ham commented Oct 7, 2019

Uh oh!

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

chorom-ham commented Oct 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects