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주차] 김류원 미션 제출합니다. #6

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 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
267 changes: 260 additions & 7 deletions package-lock.json

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"react": "^18.3.1",
"react-dom": "^18.3.1",
"react-scripts": "5.0.1",
"styled-components": "^6.1.13",
"web-vitals": "^2.1.4"
},
"scripts": {
Expand All @@ -34,5 +35,8 @@
"last 1 firefox version",
"last 1 safari version"
]
},
"devDependencies": {
"@babel/plugin-proposal-private-property-in-object": "^7.21.11"
}
}
4 changes: 4 additions & 0 deletions public/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
manifest.json provides metadata used when your web app is installed on a
user's mobile device or desktop. See https://developers.google.com/web/fundamentals/web-app-manifest/
-->
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no"
/>
<link rel="manifest" href="%PUBLIC_URL%/manifest.json" />
<!--
Notice the use of %PUBLIC_URL% in the tags above.
Expand Down
9 changes: 0 additions & 9 deletions src/App.js

This file was deleted.

122 changes: 122 additions & 0 deletions src/App.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import styled from "styled-components";
import { createGlobalStyle } from "styled-components";
import Header from "./components/Header";
import List from "./components/List";
import Editor from "./components/Editor";
import { useRef, useReducer } from "react";

const mockDate = [
{
id: 0,
isDone: false,
content: "React 공부하기",
date: new Date().getTime(),
},
{
id: 1,
isDone: false,
content: "고양이 밥 주기",
date: new Date().getTime(),
},
{
id: 2,
isDone: false,
content: "고양이 놀아주기",
date: new Date().getTime(),
},
];
Comment on lines +8 to +27
Copy link

Choose a reason for hiding this comment

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

모의 객체를 넣으신 것 같아요! (저두 류원님 고양이 만나고 싶어요🤍)

다만 보통 모의 객체는 데이터가 아직 준비되지 않은 상황이나, 서버가 연결되지 않은 상황에서 개발을 미리 진행하기 위해 넣는 것으로 알고 있습니다. 지금 투두리스트 프로젝트에서는 이미 할 일을 추가하고 삭제하는 기능이 잘 구현되어 있기 때문에 굳이 모의 객체가 있어야 하는지에 대해 살짝 의문이 듭니다! 더군다나 지금 모의 객체가 전부 고정된 데이터로 이루어져 있기 때문에 실제 해당 할 일을 하고 완료로 체크해도 새로고침하면 안 했다고 뜨게 됩니다. (is Done이 false로 고정되어 있으니까요..!) '투두리스트'라는 서비스의 목적을 고려하면 이 부분은 살짝 적절하지 않은 것 같습니다.

만약 저 세 가지 할 일을 단순히 모의 객체가 아니라 매일 하는 일들로 규정하고 싶으신 거면 아예 일반 투두리스트들과 선으로 분리해서 상단에 고정해놓으시는 것도 좋을 것 같아요! 그리고 mockDate라는 변수명은 배열의 내용을 제대로 설명하지 못하는 것 같습니다! mockTodos 등으로 수정해 보는 건 어떨까요?


function reducer(state, action) {
switch (action.type) {
case "CREATE":
return [action.data, ...state];
case "UPDATE":
return state.map((item) =>
item.id === action.targetId ? { ...item, isDone: !item.isDone } : item
);
case "DELETE":
return state.filter((item) => item.id !== action.targetId);
default:
return state;
}
}

function App() {
const [todos, dispatch] = useReducer(reducer, mockDate);
const idRef = useRef(3);

const onCreate = (content) => {
dispatch({
type: "CREATE",
data: {
id: idRef.current++,
isDone: false,
content: content,
date: new Date().getTime(),
},
});
};

const onUpdate = (targetId) => {
dispatch({ type: "UPDATE", targetId: targetId });
};

const removeTodo = (targetId) => {
dispatch({
type: "DELETE",
targetId: targetId,
});
};
Comment on lines +48 to +69

Choose a reason for hiding this comment

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

현재 위의 onCreate, onUpdate, removeTodo 함수에서는 dispatch를 내부적으로 사용하고 있네요. 제가 류원님의 생각을 다 알 수는 없지만, dispatch 를 활용하는 함수들의 선언이기에 App 컴포넌트에서 이 함수들을 만들어준 뒤, 이들을 활용하는 Editor, List 컴포넌트에 prop으로 넘겨주는 방식을 택하신 것 같아요.
물론 이 방식도 좋지만 어차피 prop 속성으로 무언가를 전달하실 것이라면 dispatch 함수를 내리고, 기능이 필요한 함수들은 컴포넌트 단에서 정의하여 사용하는 것이 더 낫다고 생각합니다.

물론 개발자가 깔끔하게 코드를 머릿 속에 넣고 있다면 좋겠지만, 다른 사람이 관련 코드를 다른 컴포넌트에 사용하고 수정한다면 관련된 컴포넌트들이 모두 영향을 받을 수도 있기 때문이에욥. 이에 관해서 나중에 얘기해보면 좋겠습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

App컴포넌트에서 상태 관리 함수가 많아질 경우 useReducer 를 활용해서 컴포넌트 외부에 따로 정리하는 것으로 알고 있었습니다! 혹시 사용기준을 잘 못 이해하고 있을까요?ㅠ

Choose a reason for hiding this comment

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

앗 아닙니다! 제가 말씀드린 의도는, useReducer() 훅 자체를 사용하시는 것은 app.jsx 파일 내에서 진행하되, onCreate, onUpdate, removeTodo 와 같은 함수들은 app 컴포넌트가 아닌 다른 컴포넌트에서 사용되고 있는 것 같아서 차라리 dispatch 함수들을 관련 컴포넌트에 넘긴 뒤, 기능을 자세히 구현한 함수를 거기에서 만들어지는 방식을 말씀드린 것이었어요.

아무래도 함수는 사용되는 컴포넌트 코드 쪽에서 선언, 활용이 모두 이루어지는 것이 나중에 가독성 측면에 있어서 더 좋다고 생각해서 그렇게 리뷰 달았습니다 😁

return (
<>
<GlobalStyle />
<AppContainer>
<Header />
<Editor onCreate={onCreate} />
<List todos={todos} onUpdate={onUpdate} removeTodo={removeTodo} />
</AppContainer>
</>
);
}

const AppContainer = styled.div`
display: flex;
flex-direction: column;
gap: 10px;
width: 500px;
margin: 0 auto;
background-color: #f1f3ff;
border-radius: 5px;
padding: 30px;
box-shadow: inset 0 0 20px #d6ddff;
height: 80vh;
box-sizing: border-box;

/* 모바일 반응형 */
@media (max-width: 768px) {
width: 360px;
height: 560px;
padding: 20px;
}
`;

const GlobalStyle = createGlobalStyle`
body {
background-color: #788bff;
margin: 0;
padding: 0;
font-family: 'Arial', sans-serif;
display:flex;
height:100vh;
justify-content:center;
align-items:center;

/* 모바일 반응형 */
@media (max-width: 768px) {
height:100vh;
width:100%;
}
}
`;
Comment on lines +114 to +120

Choose a reason for hiding this comment

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

모바일 반응형까지 고려하셨네요! 방금 폰으로 배포 링크 들어가봤는데 너무 멋집니다👍👍

Comment on lines +103 to +120

Choose a reason for hiding this comment

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

styled-components 라이브러리에서 createGlobalStyle 함수를 이용하여 전역적으로 공통 css 를 적용하는 방법을 선택해 주셨네요!
류원님께서도 배포하신 element 개발자 도구 탭을 열어보시면 알겠지만, head 태그 내부에 <style data-styled="active" data-styled-version="6.1.13"></style> 라는 태그가 생긴 것을 알 수 있어요. 오히려 소스 탭에는 따로 css 코드가 존재 하지 않는 것까지 볼 수 있네요!

추후에 저희가 nextJS 과제를 진행할 때에 이렇게 클라이언트에서 새로운 style 과 같은 요소를 만드는 방식은 네트워크 환경이 느린 경우에는 약점으로 꼽힐 수도 있다는 점을 유의하시면 좋을 것 같아요!

또한 현재의 배포본에서는 페이지가 하나이고 요소도 많지 않기 때문에 app.jsx 파일 내에서 전역 스타일링을 정해주셨는데, 애플리케이션이 커지는 경우에는 이를 분리하는 것도 괜찮다고 생각합니다 :)


export default App;
76 changes: 76 additions & 0 deletions src/components/Editor.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import styled from "styled-components";
import { useRef, useState } from "react";

const Editor = ({ onCreate }) => {
const [content, setContent] = useState("");

const contentRef = useRef();
Comment on lines +5 to +7

Choose a reason for hiding this comment

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

사용자가 아무 것도 입력하지 않은 상태에서 엔터를 눌러 입력하려고 할때, 여전히 dom 요소에 focus on 해주기 위해 useRef() 훅을, input의 양방향 바인딩을 위해 useState() 훅을 사용해주셨네요!

너무 좋은 활용법이고, 나중에 form을 다룰 때에 양방향 바인딩 외에 useRef()를 통한 방식도 있다는 것을 공부해보시면 좋을 것 같습니다. 레퍼런스 남길게요 :)


const onChangeContent = (e) => {
setContent(e.target.value);
};

const onkeydown = (e) => {
if (e.key === "Enter") {
e.preventDefault();
onSubmit();
}
};

const onSubmit = () => {
if (content.trim() === "") {
contentRef.current.focus();
return;
}
onCreate(content.trim());
setContent("");
};

return (
<EditorContainer>
<Input
ref={contentRef}
type="text"
placeholder="새로운 Todo..."
onChange={onChangeContent}
onKeyDown={onkeydown}
Copy link

Choose a reason for hiding this comment

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

onKeyUp={(e) => { if (e.key === 'Enter') { e.preventDefault(); // 기본 Enter 키 동작 막기 onSubmit(); } }}

이렇게 하시면 아마 되실 것 같아요! 그 차이가 onKeyDown, onKeyUp의 차이인데 onKeyUp은 키를 뗀 후에 트리거가 되어서 이미 input의 내용이 DOM에 적용된 후에 트리거가 발생합니다. 반대로 onKeyDown은 키를 누르는 순간에 트리거가 되기 때문에 글자가 아직 DOM에 반영되지 않았는데도 트리거가 될 수도 있는 경우가 있기에(그래서 가끔은 되는 경우도 있는 것 같습니다) 마지막 글자가 포함되는 것 같습니다!

https://codevil.tistory.com/277 참고자료도 남길게요!

Copy link
Author

Choose a reason for hiding this comment

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

헉 너무 감사합니다ㅠㅠ 이거 때매 정말 뭐가 문제지 계속 생각했었는데...
onKeyDown보다 onKeyUp을 사용해야 할 거 같네요... !!!

value={content}
/>
<Button onClick={onSubmit}>Add</Button>
</EditorContainer>
);
};

const EditorContainer = styled.div`
display: flex;
gap: 10px;
`;

const Input = styled.input`
flex: 1;
padding: 15px;
border: 1px solid rgb(220, 220, 220);

Choose a reason for hiding this comment

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

Input 컴포넌트에 border 설정이 중복해서 들어가있는 것 같습니다! border를 none으로 설정하시려면 요 라인은 삭제하셔도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

앗 제가 border를 넣고 이후에 None을 했네요ㅠㅜ 알려주셔서 감사합니다💗

border-radius: 5px;
background-color: #d6ddff;
color: #788bff;
outline: none;
border: none;
font-weight: 500;

&::placeholder {
color: #788bff;
opacity: 1;
}
`;

const Button = styled.button`
cursor: pointer;
width: 80px;
border: none;
background-color: #788bff;
color: white;
border-radius: 10px;
font-size: 1rem;
`;

export default Editor;
21 changes: 21 additions & 0 deletions src/components/Header.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import styled from "styled-components";

const Header = () => {
return (
<HeaderContainer>
<h1>TO DO</h1>
<span>{new Date().toDateString()}</span>
</HeaderContainer>
);
};

const HeaderContainer = styled.div`
h1 {
color: #788bff;
}
span {
color: #8790ca;
}
`;

export default Header;
129 changes: 129 additions & 0 deletions src/components/List.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import styled from "styled-components";
import TodoItem from "./TodoItem";
import { useState, useMemo } from "react";

const List = ({ todos, onUpdate, setTodos, removeTodo }) => {
const [search, setSearch] = useState("");

const onChangeSearch = (e) => {
setSearch(e.target.value);
};
Comment on lines +8 to +10

Choose a reason for hiding this comment

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

투두 검색 기능을 구현하신 게 정말 인상적입니다! 실사용성을 생각해봤을 때 너무 좋은 기능인 것 같아용🤗💡

Copy link

Choose a reason for hiding this comment

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

저도 검색 기능은 생각해 보지 못했던 부분이라 참신하게 느껴졌어요! 만약 투두리스트 기능이 확대되어서 월 단위나 년 단위로 저장할 수 있게 된다면, 내가 언제 이 일을 했는지, 혹은 언제 해야 하는지 검색할 때 좋을 것 같아요~~


const getFilteredDate = () => {
if (search === "") {
return todos;
}
return todos.filter((todos) => {
return todos.content.toLowerCase().includes(search.toLowerCase());
});
};
Comment on lines +12 to +19

Choose a reason for hiding this comment

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

지금 getFilteredDate 함수를 보면 검색에 따라 todos를 필터링하는 기능을 수행하는 함수로 생각됩니다! 함수명을 getFilteredDate로 설정하셔서 이름만 보고는 날짜와 관련된 기능을 수행하는 것처럼 받아들여질 수도 있을 것 같아요! 함수/변수명 작명은 개발자의 평생 숙제죠...🥹 조금 더 직관적이고 가독성 높은 코드를 위해서 함수명을 getFilteredTodos와 같이 함수 기능에 더 가깝게 네이밍해보는 건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 함수를 수정하면서 변수명도 같이 수정을 했었나봐요ㅠㅠ 함수명 연관되게 작성하도록 명심하겠습니당!!


const filteredTodos = getFilteredDate();

const { doneCount, notDoneCount } = useMemo(() => {
Comment on lines +21 to +23
Copy link

Choose a reason for hiding this comment

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

doneCount와 notDoneCount 계산 부분을 useMemo를 활용해서 효율적으로 코드를 작성해주신 것 같아요! todos 상태가 변경될 때만 계산이 일어나니까 불필요한 연산이 많이 줄어들 것 같습니다. 다만 지금 filteredTodos는 getFilteredDate() 함수에서 계속 계산되고 있는 것 같아요. 이 부분도 useMemo로 캐싱해서 todos나 search가 바뀔 때만 계산하도록 수정하면 더욱 좋을 것 같습니다..!!

Suggested change
const filteredTodos = getFilteredDate();
const { doneCount, notDoneCount } = useMemo(() => {
const filteredTodos = useMemo(() => getFilteredDate(), [todos, search]);
const { doneCount, notDoneCount } = useMemo(() => {

const totalCount = todos.length;
const doneCount = todos.filter((todo) => todo.isDone).length;
const notDoneCount = totalCount - doneCount;

return {
totalCount,

Choose a reason for hiding this comment

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

혹시 totalCount 변수까지 구조 분해할당으로 반환해주시는 이유가 있을까요? 이는 따로 받아서 사용하고 있지 않는 것 같아서요!

doneCount,
notDoneCount,
};
}, [todos]);

return (
<ListContainer>
<TopWrapper>
<h4>Todo List 🌱</h4>
<CountWrapper>
<span>todo : {notDoneCount}</span>
<span>done : {doneCount}</span>
</CountWrapper>
</TopWrapper>
<SearchInput
type="text"
placeholder="검색어를 입력하세요"
value={search}
onChange={onChangeSearch}
/>
<TodoWrapper>
{filteredTodos.map((todos) => {
return (
<TodoItem
key={todos.id}
{...todos}
onUpdate={onUpdate}
todos={todos}
setTodos={setTodos}

Choose a reason for hiding this comment

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

TodoItem 컴포넌트에 onUpdate와 removeTodo를 전달하여 투두 업데이트와 삭제를 처리하고 있기 때문에 setTodos를 전달하지 않아도 괜찮을 것 같습니다! 실제로 TodoItem 컴포넌트에서 setTodos를 사용하고 있지 않기도 하고요☺️

removeTodo={removeTodo}
/>
Comment on lines +53 to +60
Copy link

Choose a reason for hiding this comment

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

저도 같은 가빈 님과 같은 생각입니다! 지금 setTodos를 props로 내보내고 있는데, 실제 TodoItem 컴포넌트에서 setTodos를 사용하지 않고 있어요. 불필요한 props는 제거하는 게 좋을 것 같습니다.

그리고 {...todos} 부분과 todos={todos} 부분도 중복된 것 같습니다. 이미 {...todos}로 todos 안의 모든 속성들을 전달하고 있기 때문에 굳이 todos={todos}로 두 번 전달할 필요가 없어요..! 이 부분도 삭제하면 더욱 깔끔한 코드가 될 것 같습니다 🙃

Suggested change
<TodoItem
key={todos.id}
{...todos}
onUpdate={onUpdate}
todos={todos}
setTodos={setTodos}
removeTodo={removeTodo}
/>
<TodoItem
key={todos.id}
{...todos}
onUpdate={onUpdate}
removeTodo={removeTodo}
/>

);
})}
</TodoWrapper>
</ListContainer>
);
};

const ListContainer = styled.div`
display: flex;
flex-direction: column;
gap: 10px;
margin-top: 40px;
overflow-y: auto;
scrollbar-width: none;
scrollbar-width: none;
-ms-overflow-style: none;

&::-webkit-scrollbar {
display: none;
}
`;

const TopWrapper = styled.div`
display: flex;
justify-content: space-between;

h4 {
color: #788bff;
margin: 0;
}
`;

const CountWrapper = styled.div`
display: flex;
flex-direction: column;

span {
color: #8790ca;
font-size: 0.8rem;
}
`;

const SearchInput = styled.input`
width: 100%;
border: none;
border-bottom: 1px solid #d9dfff;
padding: 15px 0;
color: #788bff;
margin-bottom: 20px;
background: transparent;

&:focus {
outline: none;
color: #788bff;
border-bottom: 1px solid #788bff;
}

&::placeholder {
color: #788bff;
}
`;

const TodoWrapper = styled.div`
display: flex;
flex-direction: column;
gap: 20px;
`;

export default List;
Loading