Skip to content

[react-todo-list step1] 정창우 미션 제출합니다. #6

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

Merged
merged 3 commits into from
Jul 23, 2025

Conversation

ChangwooJ
Copy link

@ChangwooJ ChangwooJ commented Jul 15, 2025

안녕하세요. 김범수 리뷰어님! 연달아 리뷰 매칭이 되었네요 ㅎㅎ 이번 미션도 리뷰 잘부탁드립니다!

미션 내용

이번 미션은 렌더링 최적화를 해보기 위해 react에서 todo-list를 제작해보는 것이었습니다.

요구사항

  • 정해진 컴포넌트와 기능을 모두 만족하여 제작해주세요.
    각 컴포넌트 내에 정해진 기능과 역할을 준수하며 제작을 하였습니다.
TodoTemplate
├── TodoInsert
└── TodoList
    └── TodoListItem

이런 구조로 설계를 하였습니다.

제가 정확히 이해하지 못했을 수도 있을 것 같은데, 요구사항이 정확히 지켜지지 않았다면 알려주세요!

구현 영상

-.Clipchamp.5.mp4

구현 하면서...

  • 할 일 목록에 대한 저장
    할 일 데이터를 어떻게 저장해두어야하나 고민이 되었습니다. 요구사항에 상태를 통해 관리하라고 쓰여있어서, useState 혹은 전역 상태로 관리하여야 하는데, useState를 통해 관리하도록 해보았습니다.
    다 만들고보니 엄청 심하지는 않지만 그래도 props drilling이 존재하기는 해서 이럴때 Zustand 등의 전역상태관리 툴을 사용해 관리하는 것이 효과적일지 고민이 됩니다.

  • react-icon
    이런 도구가 있는 줄은 몰랐습니다. 단순히 임포트 해서 가져다 쓰기만 하면 되니 굉장히 유용하네요! 매번 귀찮게 svg파일 찾아서 파일 넣어주고 가져다 쓰고 하지 않아도 되겠습니다.

Directory Structure

image

아, 그리고 커밋을 최소 크기로 나누려고 했는데 작업하다보니 커밋하는 걸 잊어서 단위가 조금 크게 잡혔네요..

@ChangwooJ ChangwooJ requested a review from Indigochi1d July 15, 2025 15:40
Copy link
Member

@Indigochi1d Indigochi1d left a comment

Choose a reason for hiding this comment

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

창우님 또 뵙네요! 이번 미션은 어떠셨나요? 사실 이번 미션의 step1은 맛보기 미션이며 step2에서 렌더링 최적화와 함수 메모이징을 시도하기 위한 초석 정도로 생각해주셨으면 좋겠습니다!

요구사항이 단순하며 구현에 어렵지는 않았어서 딱히 말씀드릴 것은 없으나 2가지의 요구사항을 지키는 것에 조금 아쉬운 부분이 있었던 것 같아요. 혹시 따로 의도된 바가 있으셨다면 공유해주시길 바랍니다. 예를 들면, 저는 이렇게 하는게 더 효과적일거 같은데요? 와 같은 것들 말이죠!

이번 미션도 고생많으셨고 다음 미션에서 뵙는걸로 하겠습니다! 리팩토링 후에 다시 디스코드에서 멘션 걸어주세요! 수고하셨습니다!

Copy link
Member

Choose a reason for hiding this comment

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

요구사항은 다음과 같았는데요!

화면을 가운데에 정렬시켜주며, 앱 타이틀(일정관리)를 보여줌. children으로 내부 JSX를 Props로 받아와 렌더링합니다.

지금 이 형태는 children으로 내부의 것들을 렌더링하지 않고 있는 것 같아요. 혹시 이렇게 하신 특정한 이유가 있다면 들을 수 있을까요?

Copy link
Author

@ChangwooJ ChangwooJ Jul 19, 2025

Choose a reason for hiding this comment

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

생각을 잘못했습니다. 그렇게 되면 App.jsx에서부터 TodoListItem까지 props drilling이 심화될 것이라고 생각해서 템플릿에 넣기도 하였고, App.jsx에서는 무언가를 다루는 과정을 최소화 하고 싶었고, 결정적으로 당시에 children으로 내부 JSX를 받아와 렌더링 한다는게 어떤 의미인지 파악이 제대로 되지 않았습니다. 그런데 오늘 다시 생각해보니 어떤 의미였는지 이해가 되네요..

이렇게 하는게 훨씬 더 유연하게 확장 가능하군요!

src/App.jsx Outdated
<p className="read-the-docs">
Click on the Vite and React logos to learn more
</p>
<TodoTemplate />
Copy link
Member

Choose a reason for hiding this comment

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

요구사항에 따르면 TodoTemplate 의 children이 존재해야할 것 같아요!


image

또한 이 요구사항에 따르면 삭제/추가/체크 기능이 있어야 하며 App.js(jsx)에서 구현 되어야해요. 즉, 창우님의 TodoTemplate.jsx에 있는 로직들이 요구사항에 따르면 이 곳으로 옮겨와야 할 것 같습니다.
창우님의 TodoTemplate.jsx가 요구사항의 App.jsx처럼 동작하고 있네요!

Copy link
Author

Choose a reason for hiding this comment

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

이 요구 사항도 이제야 이해가 됩니다! 제 코드는 todoTemplate이 정해진 역할을 지켜주지 못하고 있기 때문에 연쇄적으로 발생한 문제군요..

요구 기능(TodoTemplate.jsx => App.jsx), TodoTemplate의 고정 방식 => 유연한 템플릿 방식
이렇게 수정해보겠습니다.

@@ -11,7 +11,9 @@
},
"dependencies": {
"react": "^18.3.1",
"react-dom": "^18.3.1"
"react-dom": "^18.3.1",
"react-icons": "^5.5.0",
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.

엇 어떤 문제가 있나요?

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.

앗 그렇군요 ㅋㅋㅋ😂

Comment on lines +4 to +13
const HeaderContainer = styled.div`
width: 100%;
height: 18%;
background-color: #007355;
display: flex;
justify-content: center;
align-items: center;
gap: 8px;
cursor: default;
`;
Copy link
Member

Choose a reason for hiding this comment

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

헤더부분을 따로 컴포넌트로 만드셨군요? 그렇다면 시맨틱 태그의 사용하는게 더 좋지 않을까요. styled.div보다는 header태그를 사용하는게 어떨까요?

Suggested change
const HeaderContainer = styled.div`
width: 100%;
height: 18%;
background-color: #007355;
display: flex;
justify-content: center;
align-items: center;
gap: 8px;
cursor: default;
`;
const HeaderContainer = styled.header`
width: 100%;
height: 18%;
background-color: #007355;
display: flex;
justify-content: center;
align-items: center;
gap: 8px;
cursor: default;
`;

Copy link
Author

Choose a reason for hiding this comment

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

그렇네요.. 같은 맥락에서
할 일 목록 전체를 감싸고 있는 TodoListContainer와 각 할 일 아이템을 감싸고 있는 TodoListItemContainer는 각각 ul li로 감싸는게 좋을까요??
또, 할 일 입력 부분을 감싸고 있는 TodoInsertContainer는 음.. form태그 등으로 감싸는건 어떤가요?

Copy link
Member

@Indigochi1d Indigochi1d Jul 21, 2025

Choose a reason for hiding this comment

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

좋네요! 엔터 키 제출도 할 수 있겠어요 👍🏻

Comment on lines +37 to +63
const TodoInsert = ({ onAddTodoItem }) => {
const [input, setInput] = useState("");

const handleKeyDown = (e) => {
if (e.key === "Enter") {
handleAddTodo(e);
}
};

const handleAddTodo = (e) => {
e.preventDefault();
onAddTodoItem(input);
setInput("");
};

return (
<TodoInsertContainer>
<TodoInput
placeholder="할 일을 입력해주세요."
value={input}
onChange={(e) => setInput(e.target.value)}
onKeyDown={handleKeyDown}
/>
<StyledAddIcon onClick={handleAddTodo} />
</TodoInsertContainer>
);
}
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 💯

Comment on lines +21 to +29
const TodoList = ({ todoList, onToggle, onDeleteTodo }) => {
return (
<TodoListContainer>
{todoList.map((todo) => (
<TodoListItem key={todo.id} todo={todo} onToggle={onToggle} onDeleteTodo={onDeleteTodo} />
))}
</TodoListContainer>
);
}
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
Member

Choose a reason for hiding this comment

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

👍🏻

@ChangwooJ
Copy link
Author

감사합니다 여전히 문장으로 되어 있는 요구사항을 이해하고 파악하는 능력이 모자라네요..
그만큼 제가 평상시에 개발 용어나 설명 등을 풀어서 이해하고 설명하고 있다는 뜻이겠죠 반성하게 됩니다..

Copy link
Member

@Indigochi1d Indigochi1d left a comment

Choose a reason for hiding this comment

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

반영된 리팩토링 사항 확인했습니다! 이번 미션은 step2가 본 미션이니 다음 미션 화이팅입니다! 물론 창우님께서는 익숙하신 내용일테지만요 😄

@ChangwooJ
Copy link
Author

리뷰 감사합니다 범수님! 😃

@Songhyejeong Songhyejeong merged commit c222a34 into greedy-team:changwooj Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants