[2주차] 김남일 미션 제출합니다. by simyeen · Pull Request #2 · CEOS-Developers/react-todo-11th

simyeen · 2020-04-10T10:59:31Z

배포주소 : https://react-todo-11th.namil.now.sh

안녕하세요! 다들 즐거운 주말되세요 :)

이번 미션을 통해서 느낀점은 확실히 저번 미션을 통해 CSS와 기본 틀짜기는 매우 숙달됬지만 아직 props 전달은 미숙하다고 생각했습니다. 특히 이번 과제에서 처음에는 input에서 data들을 바로 index로 올려주는 식으로 생각했다가 싹다 갈아엎고 다시 했습니다. index에서 input태그에 함수를 인자를 넣어주고 data를 받는 식으로 하니 확실히 전체적인 관리가 편해진 것 같습니다.또한 왜 구글링 할때마다 extend Component와 render()가 나와 참조하기 불편했었는데 지금까지 Hook를 사용하고 있는지도 모르고 계속 사용했었던거였네요...

확실히 Hook에 대해서 알고 사용해보니 전부 function Component만 쓰므로 가독성도 올라가고 Hook자체 기능또한 다양하고 좋았던 것 같습니다.처음에 todoRemove를 구성할 때는 useRef를 id로 주고 filter로 처리하려 했는데 안돼서 하는 수 없이 새로운 state를 id를 주니까는 잘 되었습니다.왜 useRef로 id를 줬을 때는 안돌아가는지 궁금하네요 ㅠㅠㅜㅜㅠㅠ 또 number타입 일때는 length기능들이 다 막혀서 String으로 변환하고 했는데 코드 돌아가는데는 딱히 상관없던 것 같습니다.

이번 과제를 통해서 state로 동적인 부분을 처리 및 어떻게 하면 이벤트들을 잘 다룰 수 있을지에 대해 공부한 것 같습니다. 아직 useEffect나 useRef, uesClick등은 미숙하지만 익숙해지면 굉장히 좋을 것 같습니다.

gywlsp

안녕하세요 남일님! 코드 리뷰어 박효진입니다.
미션 빡빡했을 수 있는데 잘 수행해주셔서 정말 감사하네요 v
수정하면 좋을 만한 사항들 리뷰로 달았으니 리뷰마다 커밋 남기시고 푸쉬 부탁드려요!

gywlsp · 2020-04-10T23:43:36Z

+import React, { useRef, useState} from 'react';

 import TodoInput from "../components/todo-input";
 import TodoList from "../components/todo-list";

 import styled from "styled-components";


Suggested change

import React, { useRef, useState} from 'react';

import TodoInput from "../components/todo-input";

import TodoList from "../components/todo-list";

import styled from "styled-components";

import React, { useState} from 'react';

import styled from "styled-components";

import TodoInput from "../components/todo-input";

import TodoList from "../components/todo-list";

import문 작성할 때 외부 라이브러리들과 직접 작성한 컴포넌트들을 구분해주시면 가독성이 좋아집니다!

useRef는 왜 쓰신 걸까요?

gywlsp · 2020-04-11T04:53:18Z

+const InputWrapper = styled.div`
+  width: 100%;
+  display: flex;
+  flex-direction: column;
+  -webkit-box-align: center;
+  align-items: center;
+  height: 37rem;
+  flex: 1 1 0%;
+  border-width: 1px;
+  border-style: solid;
+  border-color: black;
+  border-image: initial;
+  padding: 1rem 2rem;
+`;


Suggested change

const InputWrapper = styled.div`

width: 100%;

display: flex;

flex-direction: column;

-webkit-box-align: center;

align-items: center;

height: 37rem;

flex: 1 1 0%;

border-width: 1px;

border-style: solid;

border-color: black;

border-image: initial;

padding: 1rem 2rem;

`;

const Wrapper = styled.div`

width: 100%;

display: flex;

flex-direction: column;

align-items: center;

height: 37rem;

flex: 1;

border: 1px solid black;

padding: 1rem 2rem;

`;

가장 바깥 Wrapper는 굳이 다른 단어 붙이실 필요 없이 Wrapper라고 써주시면 됩니다.
어떤 역할을 하는지는 파일 이름에서 알 수 있기 때문이죠!

box-align은 표준 문법이 아니기 때문에 사용 안 하시는 게 좋습니다. 참고

flex : 1 1 0%는 flex : 1과 같습니다. 참고

border-width: 1px, border-style: solid, border-color: black를 border: 1px solid black으로 가독성 좋게 쓸 수 있습니다.

border-image는 화면 스타일을 구현하는 데에 필요 없는 코드입니다.

gywlsp · 2020-04-11T05:08:09Z

+const TimeBox = styled.div`
+  width: 100%;
+  display: flex;
+  flex-direction: row;
+  -webkit-box-pack: justify;
+  justify-content: space-between;
+  align-items: flex-start;
+  margin-bottom: 3rem;
+`;
+
+const Time = styled.p`
+  font-size: 1.5rem;
+  padding: 0px;
+  margin: 0px;
 `;
+
+const InputDay= styled.input`
+  width: 80%;
+  border-width: 1px;
+  border-style: solid;
+  border-color: rgb(97, 97, 97);
+  border-image: initial;
+  padding: 0.5rem 0.8rem;
+`;
+
+const TodoBox= styled.div`
+  width: 100%;
+  display: flex;
+  flex-direction: row;
+  -webkit-box-pack: justify;
+  justify-content: space-between;
+  align-items: flex-start;
+  margin-bottom: 3rem;
+`;
+
+const Todo = styled.p`
+  font-size: 1.5rem;
+  padding: 0px;
+  margin: 0px;
+`;
+
+const InputTodo = styled.textarea`
+  width: 80%;
+  height: 30vh;
+  resize: none;
+  border-width: 1px;
+  border-style: solid;
+  border-color: rgb(97, 97, 97);
+  border-image: initial;
+  padding: 0.5rem 0.8rem;
+`;


Suggested change

const TimeBox = styled.div`

width: 100%;

display: flex;

flex-direction: row;

-webkit-box-pack: justify;

justify-content: space-between;

align-items: flex-start;

margin-bottom: 3rem;

`;

const Time = styled.p`

font-size: 1.5rem;

padding: 0px;

margin: 0px;

`;

const InputDay= styled.input`

width: 80%;

border-width: 1px;

border-style: solid;

border-color: rgb(97, 97, 97);

border-image: initial;

padding: 0.5rem 0.8rem;

`;

const TodoBox= styled.div`

width: 100%;

display: flex;

flex-direction: row;

-webkit-box-pack: justify;

justify-content: space-between;

align-items: flex-start;

margin-bottom: 3rem;

`;

const Todo = styled.p`

font-size: 1.5rem;

padding: 0px;

margin: 0px;

`;

const InputTodo = styled.textarea`

width: 80%;

height: 30vh;

resize: none;

border-width: 1px;

border-style: solid;

border-color: rgb(97, 97, 97);

border-image: initial;

padding: 0.5rem 0.8rem;

`;

const Row = styled.div`

width: 100%;

display: flex;

flex-direction: row;

justify-content: space-between;

margin-bottom: 3rem;

`;

const Label = styled.p`

font-size: 1.5rem;

margin: 0px;

`;

const Input= styled.input`

width: 80%;

border: 1px solid rgb(97,97,97);

padding: 0.5rem 0.8rem;

`;

const Textarea = styled.textarea`

width: 80%;

height: 30vh;

resize: none;

border: 1px solid rgb(97,97,97);

padding: 0.5rem 0.8rem;

`;

TimeBox, TodoBox의 style이 같고, Time, Todo의 style이 같네요. 굳이 분리할 필요 없을 것 같아요!
합치면서 네이밍을 다음과 같이 바꾸면 좋을 것 같아요.
TimeBox, TodoBox -> Row, Time, Todo -> Label
네이밍은 그 태그가 어떤 역할을 하는지 알아보기 쉽게 하면 좋은데요,
TimeBox, TodoBox가 input/textarea의 label과 input/textarea를 같은 행에 오도록 정렬하기 때문에 Row로 바꾸면 좋을 것 같아요.
Time, Todo는 input/textarea가 어떤 값을 받는지 알려주는 태그(input/textarea의 label)이기 때문에 Label이라는 이름이 적당할 것 같아요.

InputDay, InputTodo의 네이밍을 Input, Textarea로 간결하게 만들면 좋을 것 같아요.
각 input과 textarea가 어떤 값을 받는지는 label을 통해 알 수 있기 때문에 굳이 day와 todo를 붙여줄 필요가 없습니다.
만약 나중에 저렇게 네이밍을 쓰셔야 할 경우에는, 일단 InputDay가 날짜를 받기 때문에 Day를 Date로 바꿔주시는 게 좋고요,
Date와 Todo를 받는 input이기 때문에 InputDay, InputTodo말고 DateInput, TodoInput처럼 써주시는 게 좋습니다.

box-pack은 표준 문법이 아니기 때문에 안 쓰시는 게 좋아요. 참고

border에 관한 comment는 앞의 리뷰를 참고해주세요.

p 태그에는 기본적으로 margin이 주어져 있기 때문에 padding: 0px는 필요 없는 코드로 보입니다.

gywlsp · 2020-04-11T05:29:39Z

+const EnterBtn = styled.button`
+  color : white;
+  background-color: rgb(97, 97, 97);
+  font-size: 1.5rem;
+  outline: none;
+  border-width: initial;
+  border-style: none;
+  border-color: initial;
+  border-image: initial;
+  padding: 0.5rem 1rem;
+  border-radius: 0.3rem;
+`;


Suggested change

const EnterBtn = styled.button`

color : white;

background-color: rgb(97, 97, 97);

font-size: 1.5rem;

outline: none;

border-width: initial;

border-style: none;

border-color: initial;

border-image: initial;

padding: 0.5rem 1rem;

border-radius: 0.3rem;

`;

const SubmitButton = styled.button`

color : white;

background-color: rgb(97, 97, 97);

font-size: 1.5rem;

outline: none;

border: none;

padding: 0.5rem 1rem;

border-radius: 0.3rem;

`;

input, textarea 값을 제출한다는 의미에서 enter보단 submit을 써주시는 게 좋아요.
또한 컴포넌트 네이밍은 너무 길어지지 않을 때를 제외하고는 최대한 풀어서 써주시는 게 좋습니다.
EnterBtn을 SubmitButton으로 바꿔주시면 좋을 것 같아요.

버튼의 테두리가 없기 때문에 border: none; 써주시면 될 것 같아요.

gywlsp · 2020-04-11T05:52:50Z

+export default function TodoInput({addTodo}){
+
+  const [newTodo, setNewTodo] = useState({});
+
+  const changeInputData = (e) => {
+    setNewTodo({...newTodo, [e.target.name]:e.target.value});
+  }


Suggested change

export default function TodoInput({addTodo}){

const [newTodo, setNewTodo] = useState({});

const changeInputData = (e) => {

setNewTodo({...newTodo, [e.target.name]:e.target.value});

}

export default function TodoInput({onSubmit}){

const [newTodo, setNewTodo] = useState({});

const handleFormChange = (e) => {

setNewTodo({...newTodo, [e.target.name]:e.target.value});

}

newTodo 네이밍 좋네요 👍

props를 통해 함수를 넘겨줄 때 addTodo로 보내는 것보다 onSubmit으로 보내는 것이 더 적절합니다. pages/index의 리뷰를 확인해주세요!

changeInputData보다는 handleFormChange라는 네이밍이 더 적절합니다. 참고
Input의 value가 바뀐 것보다 제출할 Form의 값이 바뀌었다는 사실이 더 중요하기 때문에 InputData를 Form으로 바꿔주시면 좋을 것 같아요!

gywlsp · 2020-04-11T07:09:35Z

+const TodoItemBox = styled.div`
+  width: 100%;
+  display: flex;
+  flex-direction: row;
+  justify-content: space-between;
+  align-items: flex-start;
+  margin-bottom: 3rem;
+  padding: 1rem 2rem;
+  border-width: 1px;
+  border-style: solid;
+  border-color: black;
+  border-image: initial;
+`;


Suggested change

const TodoItemBox = styled.div`

width: 100%;

display: flex;

flex-direction: row;

justify-content: space-between;

align-items: flex-start;

margin-bottom: 3rem;

padding: 1rem 2rem;

border-width: 1px;

border-style: solid;

border-color: black;

border-image: initial;

`;

const Row = styled.div`

width: 100%;

display: flex;

flex-direction: row;

justify-content: space-between;

margin-bottom: 3rem;

padding: 1rem 2rem;

border-width: 1px solid black;

`;

components/todo-input.js에 달았던 리뷰와 같은 이유로 네이밍을 Row로 해주시는 게 좋아요.

align-items:flex-start는 굳이 안 쓰셔도 style이 똑같을 거예용

gywlsp · 2020-04-11T07:10:15Z

+const Day= styled.p`
+  font-size: 1.5rem;
+  display: flex;
+  flex-direction: column;
+  padding: 0px;
+  margin: 0px;
+`;


Suggested change

const Day= styled.p`

font-size: 1.5rem;

display: flex;

flex-direction: column;

padding: 0px;

margin: 0px;

`;

const Date= styled.p`

font-size: 1.5rem;

display: flex;

flex-direction: column;

margin: 0px;

`;

날짜니까 네이밍은 Date가 적당할 것 같아용

gywlsp · 2020-04-11T07:11:58Z

+const ClearBtn = styled.button`
+  color: white;
+  background-color: rgb(97, 97, 97);
+  font-size: 1.5rem;
+  outline: none;
+  border-width: initial;
+  border-style: none;
+  border-color: initial;
+  border-image: initial;
+  padding: 0.5rem 1rem;
+  border-radius: 0.3rem;
 `;


Suggested change

const ClearBtn = styled.button`

color: white;

background-color: rgb(97, 97, 97);

font-size: 1.5rem;

outline: none;

border-width: initial;

border-style: none;

border-color: initial;

border-image: initial;

padding: 0.5rem 1rem;

border-radius: 0.3rem;

`;

const DeleteButton = styled.button`

color: white;

background-color: rgb(97, 97, 97);

font-size: 1.5rem;

outline: none;

padding: 0.5rem 1rem;

border: none;

border-radius: 0.3rem;

margin-left: auto;

`;

네이밍은 components/todo-input의 SubmitButton과 같은 이유로 DeleteButton으로 바꿔주시면 좋아요~

DeleteButton을 오른쪽 정렬하고 싶으시다면 margin-left: auto를 주시면 됩니다. 참고

gywlsp · 2020-04-11T07:25:17Z

+export default function TodoItem({ id, content, date, todoRemove }) {
+  return (
+    <Wrapper>
+      <TodoItemBox>
+        <Todo>{content}</Todo>
+        <Day>{date}
+        <ClearBtn onClick={(() => {todoRemove(id)})}>완료</ClearBtn>
+        </Day>
+      </TodoItemBox>
+    </Wrapper>
+    );
 }


Suggested change

export default function TodoItem({ id, content, date, todoRemove }) {

return (

<Wrapper>

<TodoItemBox>

<Todo>{content}</Todo>

<Day>{date}

<ClearBtn onClick={(() => {todoRemove(id)})}>완료</ClearBtn>

</Day>

</TodoItemBox>

</Wrapper>

);

}

export default function TodoItem({ todo, onDelete }) {

return (

<Wrapper>

<Row>

<Todo>{content}</Todo>

<Date>{date}</Date>

</Row>

<DeleteButton onClick={onDelete}>완료</DeleteButton>

</Wrapper>

);

}

props가 바뀐 건 components/todo-list.js 참고해주세요!

네이밍 바뀐 건 아래 styled-components에 달린 리뷰들 참고해주세요

TodoList가 TodoItem의 index까지 포함해서 onDelete를 넘겼기 때문에 여기에서는 onClick에 onDelete 함수만 넣어주면 잘 작동하게 됩니다.

결과 화면의 TodoItem의 구조를 나눠보면 Date 텍스트를 나타내는 태그 Date가 DeleteButton을 자식으로 가지는 게 어색합니다.
구조를 잘 나눠보면 Todo내용, 날짜가 한 행에 있고 그 아래 행 맨 오른쪽에 완료 버튼이 있기 때문에 코드를 위와 같이 작성해주시는 게 좋습니다.
DeleteButton의 style은 아래 리뷰를 참고해주세요.

최종 검토, 수정

simyeen added 6 commits April 10, 2020 13:46

component간 인자 전달

0f3b884

'todo-input.js'

c601aa0

'index.js-1'

d9ff6b3

'index.js'

72ece24

using useRef

ed2b7d7

using state for index

1106209

gywlsp suggested changes Apr 11, 2020

View reviewed changes

gywlsp pushed a commit that referenced this pull request Apr 11, 2020

Merge pull request #2 from greatSumini/sumin

19da61e

최종 검토, 수정

simyeen added 9 commits April 12, 2020 11:39

remove include #useRef

a8973c4

Modified inputWrapper -> wrapper

d3c7a30

Modified css

0f781e5

Modifed css EnterBtn -> SubmitButton

294ab06

Modified changeInputData -> handleFormChange

cf27bb9

Modified index.js

2d09456

Modified index.js

5792c81

Modified input.js

a4d9fa2

Modified All

732ba71

greatSumini changed the base branch from master to simyeen April 15, 2020 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[2주차] 김남일 미션 제출합니다.#2

[2주차] 김남일 미션 제출합니다.#2
simyeen wants to merge 15 commits into
CEOS-Developers:simyeenfrom
simyeen:namil-todo

simyeen commented Apr 10, 2020 •

edited

Loading

Uh oh!

gywlsp left a comment

Uh oh!

gywlsp Apr 10, 2020

Uh oh!

gywlsp Apr 11, 2020

Uh oh!

gywlsp Apr 11, 2020

Uh oh!

gywlsp Apr 11, 2020

Uh oh!

gywlsp Apr 11, 2020

Uh oh!

Uh oh!

gywlsp Apr 11, 2020

Uh oh!

gywlsp Apr 11, 2020

Uh oh!

gywlsp Apr 11, 2020

Uh oh!

gywlsp Apr 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

simyeen commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

simyeen commented Apr 10, 2020 •

edited

Loading