Skip to content
This repository was archived by the owner on Sep 19, 2021. It is now read-only.

[2주차] 고은 미션 제출합니다.#3

Open
eun-ko wants to merge 29 commits into
CEOS-Developers:masterfrom
eun-ko:reactEun
Open

[2주차] 고은 미션 제출합니다.#3
eun-ko wants to merge 29 commits into
CEOS-Developers:masterfrom
eun-ko:reactEun

Conversation

@eun-ko

@eun-ko eun-ko commented Apr 10, 2020

Copy link
Copy Markdown

이번주차는 저번보다 확실히 더 어렵고 시간도 많이 걸린것같습니다.
이벤트 핸들링을 다뤄볼수있어서 흥미로웠습니다.
input을 두개를 다르게 받는부분에서 시간을 많이 잡아먹었던것 같아요
아직 state에 대한 이해가 많이 부족하다는 것을 느꼈습니다.
함수형으로도 다시 만들어봐야할것같아요ㅜㅜ
->완료했습니다!
https://react-todo-11th.eun-ko.now.sh

@gywlsp gywlsp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 미션 버거울 수 있었을 텐데 잘 구현해주셔서 좋습니다 👍
수정하면 좋을 만한 것들 리뷰 달았으니 읽어보시고 리뷰마다 커밋 남기고 푸쉬 부탁드려용
자세히 설명하면 좋을 것 같아 쓰다 보니 넘 길어졌네요..ㅎ
고생하셨습니당 :)

Comment thread pages/index.js Outdated
Comment on lines +67 to +72
const Subject = styled.div`
font-size: 3rem;
font-weight: 600;
padding: 0px;
margin: 0px 0px 3rem;
color: white;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const Subject = styled.div`
font-size: 3rem;
font-weight: 600;
padding: 0px;
margin: 0px 0px 3rem;
color: white;
const Title = styled.h1`
font-size: 3rem;
font-weight: 600;
margin-bottom: 3rem;
color: white;
  1. 제목을 나타내는 태그이니까 네이밍을 Title로 하면 좋을 것 같아요.
  2. 제목을 적는 태그라서 div보단 h1을 사용하면 좋습니다.
  3. padding: 0px 코드는 꼭 필요해 보이진 않네요!
  4. 아래에만 margin을 주고 싶다면 margin-bottom: 3rem과 같이 써주시면 돼용

Comment thread pages/index.js Outdated
font-size: 0.1rem;
display: flex;
flex-direction: row;
justify-content: space-between;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
justify-content: space-between;

<Space/>를 통해 여백을 준다면 굳이 justify-content: space-between속성 안 주셔도 됩니당

Comment thread pages/index.js
Comment on lines +57 to +59
const Space = styled.div`
flex: 0.3;
`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const Space = styled.div`
flex: 0.3;
`;
const Space = styled.div`
flex: 1;
`;

flex 값을 1로 하나 0.3으로 하나 차이는 없겠지만, 보통 이런 경우 1을 많이 씁니다! 참고

Comment thread pages/index.js Outdated
Comment on lines +8 to +9
const [date, setDate] = useState();
const [todo, setTodo] = useState();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const [date, setDate] = useState();
const [todo, setTodo] = useState();

이 state들이 pages/index.js에 존재해야 할 필요가 있을까요?
components/todo-input.js에 props로 넘겨줄 필요 없이 그 컴포넌트 안에서 선언해주시면 됩니다.

Comment thread pages/index.js Outdated
const [date, setDate] = useState();
const [todo, setTodo] = useState();
const [todos, setTodos] = useState([]);
const [id, setID] = useState(0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const [id, setID] = useState(0);

id 없이 todo를 삭제할 수 있어요! 리뷰 참고해주세요. 이 state는 불필요합니당.

Comment thread components/todo-input.js
Comment on lines +13 to +37
return (
<Form>
<InputSection>
<P>시간</P>
<Input
maxlength="8"
oninput="maxLengthCheck(this)"
type="number"
placeholder="날짜를 입력하세요 (ex.20200404)"
value={date}
onChange={onChangeDate}
/>
</InputSection>

<InputSection>
<P>TODO</P>
<TextArea
placeholder="할 일을 입력하세요"
value={todo}
onChange={onChangeTodo}
/>
</InputSection>
<Button onClick={onSubmit}>등록</Button>
</Form>
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return (
<Form>
<InputSection>
<P>시간</P>
<Input
maxlength="8"
oninput="maxLengthCheck(this)"
type="number"
placeholder="날짜를 입력하세요 (ex.20200404)"
value={date}
onChange={onChangeDate}
/>
</InputSection>
<InputSection>
<P>TODO</P>
<TextArea
placeholder="할 일을 입력하세요"
value={todo}
onChange={onChangeTodo}
/>
</InputSection>
<Button onClick={onSubmit}>등록</Button>
</Form>
);
return (
<Wrapper>
<Row>
<Label>시간</Label>
<Input
type="number"
name="date"
placeholder="날짜를 입력하세요 (ex.20200404)"
onChange={handleFormChange}
/>
</Row>
<Row>
<Label>TODO</Label>
<TextArea
name="todo"
placeholder="할 일을 입력하세요"
onChange={handleFormChange}
/>
</Row>
<Button onClick={onSubmit}>등록</Button>
</Wrapper>
);
  1. self-closing 태그 잘 사용해주셨네요 👍
  2. 컴포넌트 이름 바뀐 건 아래 리뷰들을 참고해주세요.
  3. 위에 handleFormChange가 잘 작동하게 하기 위해 name attribute를 추가해줬어용
  4. onInput 문법이 존재하긴 하지만 리액트 팀에서는 이것의 사용을 추천하지 않고 있습니다. 참고
    onChange가 있는데 onInput을 또 써줄 필요는 없으므로 삭제해주시면 됩니다.
  5. inputvalue를 넣어줄 필요는 없어요! value를 써주지 않았는데 handler가 어떻게 e.target.value를 파악하는지 궁금하다면 다음을 참고해주세요. 참고

Comment thread components/todo-list.js Outdated
</Wrapper>
);
export default function TodoList(props) {
const { data, onRemove } = props;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { data, onRemove } = props;
const { todos, onDelete } = props;

왜 이렇게 바뀌었는지는 pages/index.js의 리뷰를 참고해주세용

Comment thread components/todo-list.js Outdated
Comment on lines +9 to +16
const list = data
.sort((todo1, todo2) => {
return todo1.date - todo2.date;
})
.map((todolist) => (
<TodoItem key={todolist.id} {...todolist} onRemove={onRemove} />
));
return <TodoListWrapper> {list} </TodoListWrapper>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const list = data
.sort((todo1, todo2) => {
return todo1.date - todo2.date;
})
.map((todolist) => (
<TodoItem key={todolist.id} {...todolist} onRemove={onRemove} />
));
return <TodoListWrapper> {list} </TodoListWrapper>;
return(
<Wrapper>
{todos.sort((todo1, todo2) => {
return todo1.date - todo2.date;
})
.map((todoObj, index) => (
<TodoItem key={JSON.stringify(todo)} {...{todoObj}} onDelete={onDelete(index)} />
))
}
</Wrapper>
);
  1. 가장 바깥 태그 이름은 Wrapper로 적어주시면 됩니다!
    어떤 역할을 하는지는 file 이름에서 알 수 있으니까요 :)
  2. javascript prototpe function으로만 이루어진 코드(sort(), map() 등)는 그냥 리턴 값 안에 넣어주시면 돼요!
  3. key에 id 값을 넣으셨는데 id가 사실 index 값이기 때문에 이를 key로 쓰는 건 좋지 않아요.
    그리고 id는 미션 내용을 구현하는데 필요하진 state가 아니라고 말씀 드렸죠!
    date, todo 값 모두 다른 값들과 겹칠 우려가 있어서 이런 경우에는 그냥 객체를 string으로 만들어 key로 설정해주기도 한답니다. 참고
  4. todos를 map했기 때문에 매개변수 이름은 todolist가 아닌 todo가 되어야 합니다.
    그런데 todo의 내용 property 이름을 todo라고 정의하셔서 TodoItem에서 이름이 겹치기 때문에 todoObj와 같은 이름으로 써주시면 됩니다!
    사실 property 이름을 content로 바꾸는 게 제일 좋아요 :)
  5. pages/index.js에서 handleDelete를 다음과 같이 정의했었죠.
	const handleDelete = (index) => () => {
		setTodos([...todos.slice(0,index), ...todo.slice(index+1, todos.length)]);
	};

todos를 map할 때 매개변수 index 값을 넣어서 onDelete={onDelete(index)}와 같이 전달하면

	() => {
		setTodos([...todos.slice(0,index), ...todo.slice(index+1, todos.length)]);
	};

위와 같은 함수에 index 값이 채워진 채로 TodoItem에게 전달됩니다.
그러면 자식 컴포넌트가 자신의 index를 몰라도 onDelete를 잘 사용할 수 있겠죵
6. 저번 미션 같은 경우 profile 변수만 props로 넘겨줬기 때문에 spread operator를 이용해 넘겨줬지만
지금과 같은 경우 todo와 onDelete가 엄연히 다르기 때문에 위와 같이 넘겨주는 게 좋습니다.

Comment thread components/todo-item.js Outdated
Comment on lines 5 to 16
const { id, date, todo, onRemove } = props;

return (
<TodoItemWrapper>
<Todo>{todo}</Todo>
<Date>
{date}
<Button onClick={() => onRemove(id)}>완료</Button>
</Date>
</TodoItemWrapper>
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { id, date, todo, onRemove } = props;
return (
<TodoItemWrapper>
<Todo>{todo}</Todo>
<Date>
{date}
<Button onClick={() => onRemove(id)}>완료</Button>
</Date>
</TodoItemWrapper>
);
}
const { todo, onDelete } = props;
const { date, todo } = todo;
return (
<Wrapper>
<Row>
<P>{todo}</P>
<P>{date}</P>
</Row>
<DeleteButton onClick={onDelete}>완료</Button>
</Wrapper>
);
}
  1. props, todos에서 왜 위와 같이 뽑는지는 components/todo-list의 리뷰 확인해주세용
  2. 컴포넌트 네이밍 바뀐 거는 아래 styled-component 리뷰들 참고해주세용
  3. 날짜를 나타내는 태그 안에 버튼이 있는 게 어색해요. 위와 같은 코드가 어떻게 작동하는지는 아래 stylec-component 리뷰 참고해주세용.
  4. 맨 바깥 태그는 Wrapper라고 네이밍 해주시면 됩니다.

Comment thread components/todo-item.js Outdated
Comment on lines +17 to +38
const Date = styled.div`
font-size: 1.5rem;
display: flex;
color: white;
flex-direction: column;
`;
const Todo = styled.div`
font-size: 1.5rem;
color: white;
display: flex;
flex-direction: column;
`;
const Button = styled.button`
color: white;
background-color: rgb(97, 97, 97);
font-size: 1.5rem;
outline: none;
border-width: initial;
border-style: none;
padding: 0.5rem 1rem;
border-radius: 0.3rem;
`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const Date = styled.div`
font-size: 1.5rem;
display: flex;
color: white;
flex-direction: column;
`;
const Todo = styled.div`
font-size: 1.5rem;
color: white;
display: flex;
flex-direction: column;
`;
const Button = styled.button`
color: white;
background-color: rgb(97, 97, 97);
font-size: 1.5rem;
outline: none;
border-width: initial;
border-style: none;
padding: 0.5rem 1rem;
border-radius: 0.3rem;
`;
const Row = styled.div`
display: flex;
flex-direction: row;
justify-content: space-between
`;
const P = styled.p`
font-size: 1.5rem;
color: white;
`;
const DeleteButton = styled.button`
color: white;
background-color: rgb(97, 97, 97);
font-size: 1.5rem;
outline: none;
border: none;
border-radius: 0.3rem;
padding: 0.5rem 1rem;
width: fit-content;
margin-left: auto;
`;
  1. Date, Todo의 style이 겹치네요! p 태그로 P 선언해서 써주시면 될 것 같아요 :)
  2. 버튼의 역할을 정확하게 나타내기 위해 DeleteButton으로 구체화하면 좋을 것 같아요~
  3. 결과 화면에서 TodoItem의 구조를 살펴보면,
    첫 번째 행 양 끝에 내용과 날짜가 있고, 그 다음 행 오른쪽 끝에 완료 버튼이 있죠!
    그래서 Date 태그 안에 완료 버튼을 넣는 게 어색하게 느껴져요.
    내용, 날짜를 나타내는 태그를 묶으면서 한 행에 정렬하는 Row태그 선언한 후 그 안에 P태그들 넣어주시고,
    DeleteButton에 margin-left: auto를 주어 화면과 같은 스타일을 구현할 수 있어요. 참고

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants