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주차] 이예지 미션 제출합니다. #1

Open
wants to merge 15 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
6 changes: 6 additions & 0 deletions .prettierrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"trailingComma": "es5",
"tabWidth": 2,
"semi": true,
"singleQuote": true
}
230 changes: 222 additions & 8 deletions package-lock.json

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
"@testing-library/user-event": "^13.5.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-scripts": "5.0.1",
"react-scripts": "^5.0.1",
"styled-components": "^5.3.9",
"web-vitals": "^2.1.4"
},
"scripts": {
Expand Down
Binary file removed public/favicon.ico
Binary file not shown.
4 changes: 2 additions & 2 deletions public/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<html lang="en">
<head>
<meta charset="utf-8" />
<link rel="icon" href="%PUBLIC_URL%/favicon.ico" />
<link rel="icon" href="oyat_favicon.jpg" />
Copy link
Member

Choose a reason for hiding this comment

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

이번 코드리뷰를 하며 알게됐는데, favicon 지정 시 png 등의 확장자 대신 ico 로 변환해 사용하는 게 더 좋다고 하더라구요!
링크 같이 첨부드립니닷

<meta name="viewport" content="width=device-width, initial-scale=1" />
<meta name="theme-color" content="#000000" />
<meta
Expand All @@ -24,7 +24,7 @@
work correctly both with client-side routing and a non-root public URL.
Learn how to configure a non-root public URL by running `npm run build`.
-->
<title>React App</title>
<title>Todo List ✔</title>
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
Expand Down
Binary file removed public/logo192.png
Binary file not shown.
Binary file removed public/logo512.png
Binary file not shown.
Binary file added public/oyat_favicon.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
98 changes: 94 additions & 4 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,99 @@
import React, { useState } from 'react';
import styled from 'styled-components';
import Form from './components/Form';
import Todo from './components/Todo';
import Done from './components/Done';
import GlobalStyle from './styles/GloalStyle';

const initialTodoData = localStorage.getItem('list')
? JSON.parse(localStorage.getItem('list'))
: [];
Comment on lines +8 to +10
Copy link
Member

Choose a reason for hiding this comment

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

이부분은

Suggested change
const initialTodoData = localStorage.getItem('list')
? JSON.parse(localStorage.getItem('list'))
: [];
const initialTodoData = localStorage.getItem('list')??[];

혹은

Suggested change
const initialTodoData = localStorage.getItem('list')
? JSON.parse(localStorage.getItem('list'))
: [];
const initialTodoData = localStorage.getItem('list')||[];

와 같이 적어도 괜찮을 것 같아요!

Choose a reason for hiding this comment

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

엄청난걸 배워갑니다 ... 너무깔끔해요

Copy link
Author

Choose a reason for hiding this comment

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

허걱스.... 저는 이 코드 쓰고 너무 뿌듯해 하고 있었는데... 이보다 더 간결하게 쓸 수 있었군요.... 충격적이지만 감탄... 감사합니다!!!

Copy link

@paya17 paya17 Mar 26, 2023

Choose a reason for hiding this comment

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

근데 문기님이 제시해주신 코드에서, 첫번째 코드는 로컬스토리지에 아이템이 존재하는 경우(localStorage.getItem('list')가 true)에는 그 아이템들을 initialTodoData에 할당해줘야 하니까 [ ]가 아니라 JSON.parse(localStorage.getItem('list')가 되어야 할 것 같아요! 그래서 첫번째 코드랑 두번째 코드를 합쳐서 원래 예지님 코드처럼 localStorage.getItem('list') ? JSON.parse(localStorage.getItem('list')) : [ ]로 적는 게 더 간단하지 않을까 하는 저의 작은 의견,,, 그런데 문기님처럼 로지컬 연산자(&&, ||) 활용하는 거 너무 좋은 것 같아요!!

Copy link
Member

Choose a reason for hiding this comment

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

앗 JSON.parse부분을 뺐네요 ㅜㅠ 연산자 사용해서 한줄로 굳이굳이 쓰자면 이렇게 쓸 수 있을 것 같습니다

Suggested change
const initialTodoData = localStorage.getItem('list')
? JSON.parse(localStorage.getItem('list'))
: [];
const initialTodoData = JSON.parse(localStorage.getItem('list')||"null")??[];


function App() {
const [list, setList] = useState(initialTodoData);
const [value, setValue] = useState('');

const getTodo = (todo) => {
const newTodo = {
id: Date.now(),
title: todo,
completed: false,
};
setList([...list, newTodo]);
localStorage.setItem('list', JSON.stringify([...list, newTodo]));
Comment on lines +22 to +23
Copy link

Choose a reason for hiding this comment

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

근데 바로 윗줄에서 setList로 list state를 업데이트 해줬는데, localStorage.setItem('list', JSON.stringify([...list, newTodo]));가 아니라 localStorage.setItem('list', JSON.stringify(list));로 작성하면 혹시 안되나요?! 이건 저의 단순한 궁금증입니다....!

Copy link
Author

Choose a reason for hiding this comment

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

어머 지금 그렇게 수정해서 돌려보니까 팽팽~ 잘 돌아가요!!! 예리한 지적... 감사합니닷!!! :)

};

const toggleTodo = (id) => {
let toggledTodo = list.map((data) =>
data.id === id ? { ...data, completed: !data.completed } : data
Copy link

Choose a reason for hiding this comment

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

불리언 이용해서 !로 true,false 토글하는 거 너무 좋은 것 같아요..!

Copy link
Author

Choose a reason for hiding this comment

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

히히 감사합니당👍👍

);
setList(toggledTodo);
localStorage.setItem('list', JSON.stringify(toggledTodo));
};

const deleteTodo = (id) => {
let deletedTodo = list.filter((data) => data.id !== id);
setList(deletedTodo);
localStorage.setItem('list', JSON.stringify(deletedTodo));
};

let countTodo = 0;
list.map((data) => (data.completed ? data : countTodo++));
Comment on lines +40 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.

요기는 filter사용해도 좋을 것 같아요
별 차이가 없기는 하지만, map은 약간 각각의 원소에 작업을 해주고 싶을 때사용하고, filter은 특정한 data를 가진 원소를 걸러내고싶을 때 사용하는 것 같아서요!
큰차이는 없지만, filter와 map의 performance를 비교한 글을 놓고 갑니다..!

Suggested change
let countTodo = 0;
list.map((data) => (data.completed ? data : countTodo++));
let countTodo = list.filter((data) => {data.completed}).length;

Copy link

Choose a reason for hiding this comment

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

저도 좋은 정보 알아갑니다!👍👍


return (
<div>
<h1>17기 프론트 화이팅~ 우하하</h1>
</div>
<>
<GlobalStyle />
<Conatiner>
<Form getTodo={getTodo} value={value} setValue={setValue}></Form>
<Section>
<h3>To do...😿{countTodo}</h3>
{list.map((data) =>
data.completed ? (
<></>
) : (
<Todo
Todo={data}
list={list}
setList={setList}
toggleTodo={toggleTodo}
deleteTodo={deleteTodo}
></Todo>
Comment on lines +50 to +60

Choose a reason for hiding this comment

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

completed 된 todo 만 걸러내는 작업에 js의 filter 함수를 활용해보면 어떨까요?!

Copy link
Author

Choose a reason for hiding this comment

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

filter 함수를 쓰면 더 간략하게 쓸 수 있겠네요!! 감사합니다ㅎㅎ :)

)
)}
</Section>
<Section>
<h3>Done..!😻{list.length - countTodo}</h3>
{list.map((data) =>
data.completed ? (
<Done
Todo={data}
toggleTodo={toggleTodo}
deleteTodo={deleteTodo}
></Done>
) : (
<></>
)
)}
</Section>
</Conatiner>
</>
);
}
const Section = styled.div`
flex: 0.5;
border-top: 2px solid rgb(177, 171, 171);
overflow: auto;
Copy link
Member

Choose a reason for hiding this comment

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

overflow: auto 이용해서 스크롤 잘 주셨네요! 필수는 아니지만 혹시몰라 스크롤바 커스텀 참고자료 놓고 갑니다! 스크롤바까지 커스텀하면 더 디테일이 살아날 것 같아요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

저번 주 과제에서 스크롤 바 커스텀이 가능한 걸 알았는데!! 미처 적용까지 해보진 못 했어요😿 다음 번엔 자료 참고해서 꼭 커스텀 해보겠습니당!!

padding-left: 10px;
padding: 0px 20px 15px 20px;
Comment on lines +86 to +87
Copy link
Member

Choose a reason for hiding this comment

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

아래 padding 값을 모두 지정해주었으니까 padding-left: 10px; 는 없어도 되겠네요!

Copy link
Author

Choose a reason for hiding this comment

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

허거덩... 이걸 놓쳤네요ㅠㅠㅠ 감사합니다!!!

`;
const Conatiner = styled.div`
background-color: white;
display: flex;
flex-direction: column;
height: 650px;
width: 350px;
border-radius: 20px;
box-shadow: 1px 1px 15px rgba(73, 71, 71, 0.5);
`;

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

const Done = ({ Todo, toggleTodo, deleteTodo }) => {
return (
<DoneList>
<div id="done-title">{Todo.title}</div>
<Button onClick={() => toggleTodo(Todo.id)}>🔙</Button>
<Button onClick={() => deleteTodo(Todo.id)}>✖</Button>
</DoneList>
);
};

const DoneList = styled.div`
display: flex;
#done-title {
text-decoration: line-through;
color: gray;
}
Comment on lines +16 to +19
Copy link
Member

@chaeyeonan chaeyeonan Mar 24, 2023

Choose a reason for hiding this comment

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

종종 id을 사용하는게 보이는데, 혹시 styled-component를 사용하고 있음에도 불구하고 id를 사용하시는 이유가 있을까요?

styled-component를 사용할때 className이나 id를 함께 사용하는건 권장되지 않는다고 합니다! 이미 styled-component를 사용하고 있는 만큼 className/id 대신에 컴포넌트로 관리해주면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

헉 권장되지 않는군요!! id를 사용했던 div태그도 차라리 컴포넌트로 관리해야겠네요~!! 좋은 정보 감사합니다!!👏🏻👏🏻👏🏻

`;
const Button = styled.button`
border: none;
background: none;
color: rgb(104, 87, 134);
font-size: 18px;
`;
Comment on lines +21 to +26
Copy link
Member

Choose a reason for hiding this comment

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

cursor: pointer 도 추가해주시면 마우스 동작이 더 명확하게 보일 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

우와웅 그렇군요!!! 디테일에 조금 더 신경 써야겠어요!!💪🏻

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

const Form = ({ getTodo, value, setValue }) => {
Copy link

Choose a reason for hiding this comment

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

혹시 이 컴포넌트가 아니라 App.js에 const [value, setValue] = useState(''); 작성하신 이유가 있으신가요?? 이 컴포넌트에 바로 작성해주면 굳이 App컴포넌트에서 props로 받아올 필요가 없어보여서요!

Copy link
Author

Choose a reason for hiding this comment

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

헉 이건 미처 고려하지 못 했네요...😿 Form 컴포넌트에서 작성해 주는 것이 더 효율적일 것 같아요!!

const submitTodo = (e) => {
e.preventDefault();

if (value.trim()) {
Copy link
Member

Choose a reason for hiding this comment

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

공백 예외 처리 좋네요! ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

저번 주 과제 코드 리뷰때 채연님이 알려주셔서 적용했습니다!! :)

Choose a reason for hiding this comment

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

혹시 이부분은 입력이 있을 경우를 처리하면
인덴테이션이 깊어지니까

입력이 비어있을 경우 먼저 처리해서 return으로 끊어줘도 좋을 것 같아요 😆
(실제로 카카오나 네이버 등 클린 코드 작성을 위해 인덴테이션을 최소화하라고 권장하고 있다고 해요! 😃 )

Copy link
Author

Choose a reason for hiding this comment

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

오~!! 확실히 그런 경우로 작성하면 코드 양이 훨씬 줄어들겠네요!! 좋은 아이디어 감사합니당!!😆

getTodo(value);
setValue('');
} else {
alert('Enter your todo');
}
};
const handleChange = (e) => {
setValue(e.target.value);
};
return (
<div>
<Enter>
<form onSubmit={submitTodo}>
<h2>Todo List ✔</h2>
<input
type="text"
value={value}
placeholder=" Enter your to do"
onChange={handleChange}
id="input-todo"
/>
<button type="submit" id="button">
</button>
</form>
</Enter>
</div>
Comment on lines +19 to +35
Copy link
Member

Choose a reason for hiding this comment

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

여기 최상위에 div 태그로 한번 더 감싸준 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

하이고... 무의식중에 div 태그를 막 써 버리는 습관이 고스란히 나타났네요...ㅜㅜ 알려주셔서 감사합니다!!

);
};
const Enter = styled.div`
padding: 0px 20px 15px 20px;
#input-todo {
border: none;
background-color: rgb(205, 222, 241);
border-radius: 20px;
height: 35px;
font-size: 15px;
width: 85%;
}
#button {
border: none;
background: none;
font-size: 18px;
width: 40px;
}
Comment on lines +40 to +53
Copy link
Member

Choose a reason for hiding this comment

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

마찬가지로 styled-component를 이용해봅시다!

`;

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

const Todo = ({ Todo, toggleTodo, deleteTodo, title, list, setList }) => {
const [isEditing, setIsEditing] = useState(false);
const [editedTitle, setEditedTitle] = useState(title);

const editTodo = (e) => {
setEditedTitle(e.target.value);
};

const submitEditedTodo = (event) => {
event.preventDefault();

let editedTodo = list.map((data) => {
if (data.id === Todo.id) {
data.title = editedTitle;
}
return data;
});
Comment on lines +15 to +20

Choose a reason for hiding this comment

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

Map을 활용한 센스가 진짜 좋은 것 같아요 👍👍


setList(editedTodo);
Comment on lines +15 to +22
Copy link

Choose a reason for hiding this comment

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

예지님이 App.js에 toggleTodo, deleteTodo 함수 만들어주셔서 list 상태를 업데이트해주신 것처럼, 이 부분도 굳이 이 컴포넌트로 list와 setList를 props로 받아오지 않고, App.js에 list 상태를 업데이트해주는 editTodo 함수를 만들어주고 이 컴포넌트에서 수정된 todo를 editTodo함수의 인자로 전달하는 방식을 사용하면 코드가 더 깔끔해질 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

허걱 저는 시야가 좁았나봐요...😿 그 방법이 더 효율적이고 가독성도 높을 것 같아요!!! 좋은 방법 알려주셔서 감사합니다!!

setIsEditing(false);
localStorage.setItem('list', JSON.stringify(editedTodo));
};

if (isEditing) {
return (
<TodoList>
<div>
<form onSubmit={submitEditedTodo}>
<input value={editedTitle} onChange={editTodo} id="edit-todo" />
</form>
</div>
</TodoList>
);
} else {
return (
<TodoList>
<div>
{Todo.title}
<Button onClick={() => toggleTodo(Todo.id)}>✔</Button>
<Button onClick={() => deleteTodo(Todo.id)}>✖</Button>
<Button onClick={() => setIsEditing(true)}>🛠</Button>
</div>
Comment on lines +40 to +45
Copy link
Member

Choose a reason for hiding this comment

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

여기도 마찬가지로 div로 한번 더 감싸주신 이유가 궁금하네요!

</TodoList>
);
}
};

const TodoList = styled.div`
display: flex;
#edit-todo {
height: 20px;
font-size: 15px;
}
`;

const Button = styled.button`
border: none;
background: none;
color: rgb(104, 87, 134);
font-size: 18px;
`;
Comment on lines +59 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.

나중에 팀프로젝트 할때는 이렇게 공통 컴포넌트가 쓰일 경우 스타일 파일을 새로 만들어서 재사용할 수 있게끔하여 중복되는 코드를 줄이는 것도 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

헉 지금 보니까 그렇게 하는 게 더 효율적이겠네요!! 좋은 정보 감사합니당ㅎㅎ😻


export default Todo;
11 changes: 4 additions & 7 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';
import { createRoot } from 'react-dom/client';

ReactDOM.render(
<React.StrictMode>
<App />
</React.StrictMode>,
document.getElementById('root')
);
const rootElement = document.getElementById('root');
const root = createRoot(rootElement);
root.render(<App />);
21 changes: 21 additions & 0 deletions src/styles/GloalStyle.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { createGlobalStyle } from 'styled-components';

Choose a reason for hiding this comment

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

우와 ... styled-component 에서
전역으로 스타일을 지정하는 방법이 있는지 처음 알았네요..🥺 전 이걸 몰라서 App.css 에 항상 전역 스타일링 코드를 작성했었거든요..! 이렇게 배워갑니다 👍👍👍

Copy link
Author

Choose a reason for hiding this comment

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

저도 이번 과제 하면서 새롭게 알게 됐어요!! 확실히 전역으로 관리하니까 편리하고 가독성도 높아지더라구요!! 제가 참고했던 자료입니다ㅎㅎ GlobbalStyle

Copy link
Member

Choose a reason for hiding this comment

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

저도 이 부분은 처음 알았어요! 이번에 전역적인 스타일은 index.css에 작성했는데, styled-components에서 createGlobalStyle 을 사용할 수 도 있다는 것 배워갑니다!

Copy link

Choose a reason for hiding this comment

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

우와 저도 유용한 정보 알아가요!!❤


const GlobalStyle = createGlobalStyle`
@font-face {
font-family: 'GangwonEdu_OTFBoldA';
src: url('https://cdn.jsdelivr.net/gh/projectnoonnu/[email protected]/GangwonEdu_OTFBoldA.woff')
format('woff');
font-weight: normal;
font-style: normal;
}
body {
background-color: rgb(186, 191, 225);
height: 100vh;
display: flex;
justify-content: center;
align-items: center;
font-family: 'GangwonEdu_OTFBoldA'
}
`;

export default GlobalStyle;