[FEAT] Todo 실습 과제 #1
Conversation
There was a problem hiding this comment.
안녕하세요 다미님. 리뷰드립니다 :)
데이터 상태관리 부분과 UI는 분리할 수 있다면, 최대한 분리하는게 좋다고 생각합니다.
코드를 보며, 제 코드에서도 리펙토링 할 점들이 보여 많이 배워가는 시간이었습니다.
세부적인 리뷰사항은 아래 코드에 리뷰했습니다. 좋은 하루 보내세요 :)
list들이 가운데 정렬이 되는데, 기본 CRA css 때문인것같아요.
remove 부분을 아래처럼 상태에 직접 접근하여 수정하였는데, 상태를 이런식으로 재할당하여도 문제가 없어서 의아하네요..
어떤 점이 문제가 된다고 생각하셨나요?
interface 변수 정의를 할 때 변수명앞에 I나 T를 붙이곤 하는데요 호찬님은 어떤식으로 작성하시는지 궁금합니다.
저는 I를 붙입니다. 하지만, 클린 코드책에서는 I를 권장하지 않는다고 해서, 어떤 방식이 좋을지 고민하고 있습니다. (생각해봐도 저는 I가 좋아보입니다 ㅎㅎ..)
repository 개념도 넣어보려하였으나 아직은 불필요하여 구현되지 않은 상태입니다.
repository가 mobx에서 ajax 부분이여서, 따로 저도 불필요하다고 생각합니다. 다만, 저는 model 개념을 어떻게 사용하면 좋을지 고민하고 있습니다.
브랜치 명은 원래 dami/feat을 의도하였으나 적용되지 않아 dami-feat으로 작성한 점 참고 부탁드려요.
넵넵! 금욜날 말씀드린것과 같이, 추후에 merge를 하려고한다면, 폴더명을 변경하거나, 따로 클론해서 PR날리는것도 좋은것 같아요 :)
| const App = () => { | ||
| return ( | ||
| <div className="App"> | ||
| <Main /> |
There was a problem hiding this comment.
하위 컴포넌트가 TodoInput와 TodoList 인만큼 Main보다는 두 컴포넌트를 포괄할 수 있는 이름이 좋을것 같아요!
| import TodoStore from "../../stores/TodoStore"; | ||
| import { useState } from "react"; | ||
|
|
||
| interface ITodo { |
There was a problem hiding this comment.
항상 고민인 부분인데, interface를 하나의 폴더에 몰아서 선언을 하는게 좋을지, 해당 파일에 선언하는게 좋을지 고민이 있습니다. 쓰다보면, 다른 함수를 정의할 때, 동일한 interface를 써야하는 경우가 많은데, 그 함수에 맞게 새로 정의를 해주는게 좋을지 파일로 분리시켜서 한눈에 보기 좋을지...
각각의 장단점이 있어보여요.
- 각각의 파일에서 interface를 정의
- 독립적인 컴포넌트로 분리하기 쉽다.
- 일관성이 떨어진다. (가장 좋은 예는 현재 코드에서 같은 자료구조에 대해, ITodoData와 ITodo에서 동일한 의미가 checked와 completed, text와 content로 나뉘어있습니다.)
- 하나의 파일에서 interface를 몰아서 정의
- 전체 interface를 파악하기 좋다.
- 일관성이 있다.
- 컴포넌트가 하나의 interface에 종속적이다.
다미님 생각은 어떠하신가요?
export interface ITodoData {
todoId: number;
content: string;
completed: boolean;
}interface ITodo {
text: string;
checked: boolean;
id: number;
}/src/types/TodoType.ts 에서 interface에 대한 파일도 있네요. 두 방식을 모두 혼용하는건 좋지 않다고 생각합니다..!
There was a problem hiding this comment.
아 이부분은 늘 고민인 내용이었습니다. 저는 컴포넌트 간에 공통적으로 사용하는 것은 types에 분리하여서 사용하고, 각 컴포넌트 내부에서 만 사용되는 부분은 공통 types을 사용해서 type을 확장해서 사용하거나, 해당 컴포넌트 내 type을 별도로 정의해서 사용했습니다. 이렇게 하게되면 하나의 파일에서 interface를 사용하는 것과 각각 interface를 관리하는 것 두 방법이 가진 장점을 살릴 수 있다고 생각하는데, 이런 방식으로 혼용하는 것도 지양해야 할까요?? 어렵네요..
There was a problem hiding this comment.
장단점에 대해서 자세히 남겨 주셨네요 감사합니다~!
There was a problem hiding this comment.
저는 컴포넌트 간에 공통적으로 사용하는 것은 types에 분리하여서 사용하고, 각 컴포넌트 내부에서 만 사용되는 부분은 공통 types을 사용해서 type을 확장해서 사용하거나, 해당 컴포넌트 내 type을 별도로 정의해서 사용했습니다.
두 방식 모두 장단점이 있지만, 혼용의 경우 일관성이 사라져서 좋지 못하다고 생각합니다~!
|
|
||
| const handleCheckedTodo = () => setIsChecked(!isChecked); | ||
| const handleDeleteTodo = () => TodoStore.removeTodo(id); | ||
|
|
There was a problem hiding this comment.
오.. 클린코드 실천하셨군요! 공백 너무 좋아 보입니다.
| const TodoItem = ({ text, checked, id }: ITodo) => { | ||
| const [isChecked, setIsChecked] = useState(checked); | ||
|
|
||
| const handleClick = (e: React.MouseEvent<HTMLElement>) => { |
| const getClassName = () => (isChecked ? "checked-todo" : "unchecked-todo"); | ||
|
|
||
| return ( | ||
| <div className="todo-container" onClick={handleClick}> |
There was a problem hiding this comment.
ul 하위의 한 항목에 대한 컴포넌트니까, 따로 wrapper를 만들기보다는 li를 wrapper로 쓰는게 좋아보이는데 어떻게 생각하시나요?
There was a problem hiding this comment.
아 이부분은 text클릭 이벤트와 x 부분 클릭 이벤트를 각각 등록하는 것이 아니라, wrapper에만 이벤트를 등록해서 이벤트 위임 방식으로 이벤트를 핸들링 하려는 목적으로 생성한 부분인데요! 말씀해주신 것 처럼 외부에서 ul이 감싸는 형태로 되어있으니 이 list가 이 컴포넌트를 감싸며 렌더링 하는 형식으로 바꾸는 것이 좋겠네요 감사합니다 !
| {text} | ||
| </div> | ||
| </li> | ||
| <span className="todo-delete-btn">❌</span> |
There was a problem hiding this comment.
누르는 요소기 때문에, span보다는 의미있는 button이 좋아보입니다.
|
|
||
| const TodoList = () => { | ||
| const { todoData } = TodoStore; | ||
|
|
There was a problem hiding this comment.
TodoItem에 대한 이벤트 핸들러들이 에 선언되어 있는데, 이렇게 되면 매번 새로 정의가 되어서 그것보다는 TodoList에서 정의를 한 뒤 전달해주는게 좋다고 생각합니다.
There was a problem hiding this comment.
props로 전달하거나 context API 를 적용하는게 낫겠군요 감사합니다!
| } | ||
|
|
||
| const TodoStore = observable<ITodo>({ | ||
| todoData: [], |
There was a problem hiding this comment.
mobx에서 해당 변수를 observable로 등록하지 않으면, 최적화가 이뤄지지 않습니다. 메소드도 action으로 등록해줘야합니다.
There was a problem hiding this comment.
store를 class가 아닌 object로 만들면 그 obejct를 observable로 감싸주기만 하면 되더라고요. 사실 아직 어떤 더 효율적인지 모르겠지만, 이번엔 호찬님 코드 통해서 observable 뿐만 아니라 makeObservable 사용하는 방법도 알게되었네요👍👍
There was a problem hiding this comment.
앗앗 observable<ITodo>() 로 감싸져 있었군요! 제가 공부가 부족했습니다. 여러 방식이 있다보니 혼동했어요. 각각 방식의 차이점을 알아봐야겠어요. 감사합니다 👍👍
| }; | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
input과 button는 관련있는 form역할을 수행하고 있습니다. 의미없는 <> 보다는 다미님의 다른 코드 부분인
<div className="todo-container" onClick={handleClick}>
와같이 의미있는 단위로 묶어주면 좋을것같아요. 또, TodoInput보다는 button의미도 포괄할 수 있는 네이밍이면 좋을것 같아요 :)
There was a problem hiding this comment.
오.. button의 의미를 포괄하는 이름을 짓는다는 건 생각 못해봤는데 좋은 생각이네요 감사합니다! 👍
| const Main = () => { | ||
| return ( | ||
| <> | ||
| <TodoInput /> |
There was a problem hiding this comment.
TodoInput과 TodoList의 개념을 포괄할 수 있는 wrapper component가 있으면 더 좋지 않을까요?
📌 개요
👩💻 작업 사항
✅ 참고 사항