Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2주차] 최민주 미션 제출합니다. #9

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
243 changes: 236 additions & 7 deletions package-lock.json

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
"@testing-library/user-event": "^13.5.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-icons": "^4.8.0",
"react-scripts": "5.0.1",
"styled-components": "^5.3.9",
"web-vitals": "^2.1.4"
},
"scripts": {
Expand Down
3 changes: 3 additions & 0 deletions src/App.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
body{
background-color: black;
}
178 changes: 175 additions & 3 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,180 @@
import {React, useState} from 'react'

Choose a reason for hiding this comment

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

저도 이렇게 사용했었는데 React를 적어주지 않아도 되는 걸 이번 코드 리뷰 하면서 처음 알게 됐어요!!
예린님 과제에서 문기님이 코드 리뷰 해주신 부분 참고하시면 도움 되실 것 같아요!!! 코드 리뷰

import './App.css'
import styled from 'styled-components';
import TodoInput from "./components/TodoInput.js";
import TodoList from "./components/TodoList.js";
import DoneList from "./components/DoneList.js";
let getId = 1;

const Container = styled.div`
background-color: black;
width: 30rem;
margin: auto;
height: 50rem;
border-style: solid;
border-radius: 2%;
border-color: white;
`

const Title = styled.div`
width: 90%;
margin-left: 5%;
margin-top: 3rem;
margin-bottom: 1.5rem;
Comment on lines +21 to +23
Copy link
Member

Choose a reason for hiding this comment

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

  1. 각각 따로 지정하고 싶으실 경우 margin: 5px 10px 15px 20px 이렇게 한 줄로 작성하시면 더 좋을 것 같아요! 참고자료 놓고 갑니다
  2. %rem을 혼용해서 쓰셨는데 혹시 이유가 있을까요?

height: 5vh;
color: white;
font-size: 30px;
text-align: center;
font-weight: 800;

border-bottom: solid;
border-radius: 2%;
`

const TodoTitle = styled. div`
width: 82%;
height: 32px;
color: white;
font-size: 28px;
text-align: left;
font-weight: bold;
margin-left : 15%;
margin-top : 1.5rem;
margin-bottom: 4px;
display: flex;
`
const DoneTitle = styled. div`
width: 82%;
height: 32px;
color: white;
font-size: 28px;
text-align: left;
font-weight: bold;
margin-left : 15%;
margin-top : 1.5rem;
margin-bottom: 4px;
display: flex;
Comment on lines +34 to +56
Copy link
Member

Choose a reason for hiding this comment

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

현재 TodoTitleDoneTitle의 스타일이 동일한데 이럴 경우 굳이 컴포넌트를 나누지 않고 하나의 컴포넌트로 사용하는게 코드를 줄일 수 있을 것 같네요!

+) 밑에 있는 컴포넌트들도 중복된게 많아보이는데 하나의 컴포넌트로 사용해주세요!

`
const TodoBox = styled.div`
position: relative;
width: 82%;
height: 30%;
margin-left: 8%;
margin-top: 1rem;
`
Comment on lines +58 to +64
Copy link
Member

Choose a reason for hiding this comment

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

할 일의 개수가 box 크기보다 많아졌을 경우 처리가 안되어서 밑의 영역까지 넘어가게 되는데, overflow: auto 사용해서 스크롤 적용해주시는게 좋을 것 같아요!


const DoneBox = styled.div`
position: relative;
width: 82%;
height: 30%;
margin-left: 8%;
margin-top: 1rem;
`
const TodoRldBtn = styled.button`
margin-left: 50%;
border: none;
width: 60px;
font-size : 25px;
background-color : black;
`
const DoneRldBtn = styled.button`
margin-left: 50%;
border: none;
width: 60px;
font-size : 25px;
background-color : black;
`

function App() {
const [todos, setTodos] = useState([]);
const addTodoList = (text) => {
if (text === ""){ //trim 나중에 적용시키기
return alert("입력된 내용이 없습니다.");
} else{
const todo = {
id: getId,
text,
checked: false
}
setTodos(todos => todos.concat(todo));

Choose a reason for hiding this comment

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

concat 함수보다 push나 ...(spread) 연산자를 사용하시는 것도 좋을 것 같아요!! 이것도 문기님 코드 리뷰 보고 알았습니당!! 참고하시면 좋을 것 같아요💪🏻💪🏻 코드 리뷰

Copy link

Choose a reason for hiding this comment

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

예지님 말씀처럼 spread(...) 연산자를 사용하는 게 좋을 것 같아요!

Suggested change
setTodos(todos => todos.concat(todo));
setTodos([...todos,todo]);

getId++;
}
};
const [dones, setDones] = useState([]);
const addDoneList = (id, text) => {
const done = {
id,
text,
checked: false
}
setDones(dones => dones.concat(done));
}
const onRemove = id => {
setTodos(todos => todos.filter(todo => todo.id !==id))
}
const onRemove_dn = id =>{

Choose a reason for hiding this comment

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

함수명에 camelCase와 underscore를 혼용하셨는데 camelCase로 통일하는건 어떨까요~?

setDones(dones => dones.filter(done => done.id !==id))
}
Comment on lines +112 to +117
Copy link
Member

Choose a reason for hiding this comment

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

코드가 중복되어 보이는데 하나의 함수로 만들면 어떨까요!

// const [virtuals, setVirtuals] = useState([]);
// const virtualList = (id, text, checked) =>{
// const virtual = {
// id,
// text,
// checked
// }
// setVirtuals(virtuals => virtuals.concat(virtual));
// }

const onChecked = (id) =>{
setTodos(todos =>
todos.map(todo =>
todo.id===id ? {...todo, checked: !todo.checked} : todo,
)
);
// reArray(todos);



};

const reArray = (todos) =>{
// const virtuals = todos;
// setVirtuals(virtuals => virtuals.filter(virtual=> virtual.checked !==true))
// console.log(virtuals);
// setTodos(todos => todos.filter(todo => todo.checked !== false))
// console.log(todos);
// setTodos(todos =>
// todos.map(todo =>
// todo.checked===true ? {...todo, id: id+5} : todo,
// )
// );
// setTodos(todos => todos.concat(virtuals))
// console.log(todos);
// setVirtuals([])
}

return (
<div>
<h1>17기 프론트 화이팅~ 우하하</h1>
</div>
<body>
<Container>
<Title>T O - D O &ensp;L I S T</Title>
<TodoInput
addTodoList={addTodoList}
/>
<TodoTitle>To-Do ({todos.length})<TodoRldBtn onClick = {() => {setTodos([])}}>🔄</TodoRldBtn></TodoTitle>
<TodoBox>
<TodoList todos={todos}
onRemove={onRemove}
addDoneList={addDoneList}
onChecked = {onChecked}/>
</TodoBox>
<DoneTitle>Done ({dones.length})<DoneRldBtn onClick = {() => {setDones([])}}>🔄</DoneRldBtn></DoneTitle>

Choose a reason for hiding this comment

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

버튼 누르면 한번에 초기화시키는 기능 좋네요 ㅎㅎ

<DoneBox>
<DoneList dones={dones}
onRemove_dn={onRemove_dn}
addTodoList={addTodoList}/>
</DoneBox>
</Container>
</body>
Comment on lines +157 to +177
Copy link
Member

Choose a reason for hiding this comment

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

혹시 여기 body 태그는 왜 작성하셨는지 이유가 궁금합니다!
App.css에 적용한 css때문이라면 Container에도 같은 속성이 적용되어 있어서 굳이 써주지 않아도 될 것 같아요~

);
}

Expand Down
62 changes: 62 additions & 0 deletions src/components/DoneItem.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import React from 'react';
import styled from 'styled-components';

const TextBox = styled.div`

Choose a reason for hiding this comment

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

주로 div를 사용하셨는데 TextBox의 경우 h1 같이 의미를 담고 있는 태그들을 사용하는게 가독성이 좋을거 같아요!!

width: 100%;
border-color: white;
color: white;
font-size: 22px;
font-weight: medium;
height: 23px;
margin: 0;
`
const DoneBox = styled.div`
position: relative;
Copy link

Choose a reason for hiding this comment

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

position속성도 좋지만 flex-box도 많이 활용해보셨으면 좋겠어요! 정말 편리한 아이랍니다ㅎㅎ

display: flex;
width: 90%;
margin-left: 10%;
margin-top: 0.7rem;
`
const RtnBtn = styled.div`
background-color: black;
border: 1.5px solid red;
border-radius: 5%;
height: 23px;
width: 60px;
color : red;
font-size: 19px;
font-weight: medium;
margin-right: 25px;
text-align: center;
`
const DltBtn = styled.div`
background-color: black;
border: none;
border-radius: 5%;
height: 22px;
width: 22px;
color : white;
font-size: 20px;
font-weight: medium;
margin-left : 10px;
Comment on lines +20 to +41
Copy link
Member

Choose a reason for hiding this comment

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

RtnBtnDltBtn처럼 완전히 동일한 css 속성은 아니지만 비슷한 속성이 많을 경우 props를 이용하면 효율적으로 코드 작성이 가능합니다!
참고자료 놓고 갑니다

`

const DoneItem = ({done, onRemove_dn, addTodoList})=>{
const {id, text} = done;
const onReturn = (id, text) =>{
onRemove_dn(id);
addTodoList(text);

}

return (
<DoneBox>
<RtnBtn onClick={() => onReturn(id, text)}>복귀</RtnBtn>
<TextBox>{text}</TextBox>
<DltBtn onClick={() => onRemove_dn(id)}>X</DltBtn>
</DoneBox>
);

}

export default DoneItem;
15 changes: 15 additions & 0 deletions src/components/DoneList.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import React from 'react';
import DoneItem from './DoneItem';

const DoneList = ({dones, onRemove_dn, addTodoList}) =>{
return (
<div>
{dones.map(done =>(
<DoneItem done={done} key = {done.id} onRemove_dn = {onRemove_dn} addTodoList={addTodoList}/>
// 아 뭔가 이렇게 이중으로 안 넘기고 바로 지우는 방법도 있을 것 같은데...
Comment on lines +7 to +9
Copy link
Member

Choose a reason for hiding this comment

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

DoneItem의 리스트를 따로 컴포넌트로 만들지 않고 App.js에 바로 map문을 적용하면 한번 더 props로 넘기는 과정을 생략할 수 있을 것 같네요!

))}
</div>
);
}

export default DoneList;
61 changes: 61 additions & 0 deletions src/components/TodoInput.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import {React, useState} from 'react';
import styled from 'styled-components';

const InputBox = styled.div`
margin-left: 10%;
margin-top: 1rem;
width:80%;
height: 35px;
border-style: solid;
border-width: 1px;
border-color: white;
Comment on lines +9 to +11
Copy link

Choose a reason for hiding this comment

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

이렇게 한 줄로 적을 수 있어요!

Suggested change
border-style: solid;
border-width: 1px;
border-color: white;
border: 1px solid white;

position: relative;
display: flex;
`
const TextInput = styled.input`
width: 90%;
height: 32px;
text-align: center;
background-color: black;
font-size: 15px;
color: gray;
border-style:none;
`
const InputButton = styled.button`
position: relative;
background-color: white;
border: solid 0.5px white;
width: 10%;
height: 35px;
text-align: center;
color: black;
font-size: 28px;
`


const TodoInput = ({addTodoList}) => {
const [value, setValue] = useState("");
const onChange = e => {
setValue(e.target.value);
}

const onSubmit = (e) =>{
e.preventDefault();
addTodoList(value);
setValue("");
}
return(
<form onSubmit={onSubmit}>
<InputBox>
<TextInput
placeholder="&emsp;할 일을 입력해주세요"
value={value}
onChange={onChange}
></TextInput>
<InputButton>+</InputButton>
</InputBox>
</form>
);
}

export default TodoInput;
Loading