Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 React와 TypeScript를 사용하여 기본적인 할 일(Todo) 애플리케이션을 완성하는 미션의 결과물입니다. 핵심 CRUD 기능 구현, 외부 더미 API 연동을 통한 데이터 초기화, React Context API를 활용한 효율적인 상태 관리, 그리고 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| useEffect(() => { | ||
| fetch('https://dummyjson.com/todos') | ||
| .then(res => res.json()) | ||
| .then(data => setTodos(data.todos)) |
There was a problem hiding this comment.
초기 데이터를 처리하는 방식에 두 가지 심각한 문제가 있습니다:
date속성 누락:dummyjsonAPI 응답에는Form인터페이스에 필요한date필드가 포함되어 있지 않습니다. 이로 인해formatDate가 호출될 때 런타임 오류가 발생할 수 있습니다. 가져온 각 todo 항목에date를 추가해야 합니다.- ID 충돌:
lastId가 올바르게 초기화되지 않았습니다. 새 todo를 추가할 때 중복 ID가 생성되는 것을 방지하려면 가져온 데이터의 최대 ID를 기반으로 설정해야 합니다.
두 문제를 모두 해결하기 위한 제안입니다.
.then(data => {
const initialTodos = data.todos.map((todo: any) => ({ ...todo, date: Date.now() }));
setTodos(initialTodos);
if (initialTodos.length > 0) {
lastId.current = Math.max(...initialTodos.map((t: Form) => t.id)) + 1;
}
})
| return ( | ||
| <ul> | ||
| {filteredTodos.map(e => ( | ||
| <TodoItems todos={e}/> |
There was a problem hiding this comment.
React에서 리스트를 렌더링할 때는 각 항목을 고유하게 식별하기 위해 key prop이 필요합니다. 이는 성능에 중요하며, 리스트 내에 상태를 가진 컴포넌트가 있을 때 버그를 방지합니다. <TodoItems /> 컴포넌트에 todo의 ID를 사용하여 고유한 key를 추가해주세요.
| <TodoItems todos={e}/> | |
| <TodoItems key={e.id} todos={e}/> |
| <Link to="/list/active">진행중인 목록</Link> | ||
| <Link to="/list/completed">완료된 목록</Link> | ||
| </nav> | ||
| <div className="b2"> |
There was a problem hiding this comment.
CSS 클래스 이름 b2는 의미를 알기 어렵습니다. 클래스에 content-wrapper나 main-content처럼 의미 있는 이름을 사용하면 코드를 읽고 유지보수하기가 더 쉬워집니다.
| <div className="b2"> | |
| <div className="main-content"> |
| interface ItemProps { | ||
| todos: Form; | ||
| } | ||
|
|
||
| const TodoItems = ({todos}:ItemProps) => { |
There was a problem hiding this comment.
이 컴포넌트의 todos prop은 단일 todo 항목을 나타내지만 이름이 복수형이라 오해의 소지가 있습니다. 가독성을 높이기 위해 단수형인 todo로 이름을 바꾸는 것이 좋습니다. 또한 컴포넌트 내에서 todos.를 todo.로 모두 수정해야 합니다.
| interface ItemProps { | |
| todos: Form; | |
| } | |
| const TodoItems = ({todos}:ItemProps) => { | |
| interface ItemProps { | |
| todo: Form; | |
| } | |
| const TodoItems = ({todo}:ItemProps) => { |
|
|
||
|
|
||
| const addTodo = (text: string) => { | ||
| setTodos([{id: lastId.current, todo:text , completed:false, date: Date.now()}, ...todos]) |
There was a problem hiding this comment.
이전 상태를 기반으로 상태를 업데이트할 때는 setState의 함수형 업데이트 형태를 사용하는 것이 더 안전합니다. 이렇게 하면 항상 최신 상태를 기반으로 작업하게 되어 잠재적인 경쟁 상태를 피할 수 있습니다.
setTodos(prevTodos => [{id: lastId.current, todo:text , completed:false, date: Date.now()}, ...prevTodos])
| div { | ||
| display: flex; | ||
| flex-direction: row; | ||
| } |
There was a problem hiding this comment.
div와 같은 광범위한 요소 선택자를 스타일링에 사용하면 애플리케이션 전체에 의도하지 않은 부작용을 일으킬 수 있습니다. 특히 컴포넌트 기반 아키텍처에서는 충돌을 피하기 위해 특정 클래스 이름을 사용하여 스타일의 범위를 지정하는 것이 좋습니다.
| import InputForm from "../component/InputForm.tsx"; | ||
|
|
||
| const Form = () => { | ||
|
|
||
| return ( | ||
| <InputForm></InputForm> | ||
| ) | ||
| } | ||
|
|
||
| export default Form; No newline at end of file |
There was a problem hiding this comment.
이 Form 컴포넌트는 InputForm 컴포넌트를 감싸는 역할만 합니다. 이는 불필요한 추상화 계층을 추가합니다. 이 컴포넌트를 제거하고 Home.tsx에서 InputForm을 직접 사용하면 코드를 단순화할 수 있습니다.
| @@ -0,0 +1,7 @@ | |||
|
|
|||
| export interface Form { | |||
There was a problem hiding this comment.
인터페이스 이름 Form은 폼이 아닌 단일 "todo" 항목을 나타내므로 혼동을 줄 수 있습니다. Todo로 이름을 변경하면 코드를 더 직관적이고 이해하기 쉽게 만들 수 있습니다. 이 이름 변경은 이 타입을 사용하는 다른 파일들의 업데이트를 필요로 합니다.
| export interface Form { | |
| export interface Todo { |
| @@ -0,0 +1,12 @@ | |||
|
|
|||
| const formatDate = (date) => { | |||
There was a problem hiding this comment.
date 매개변수는 암시적으로 any 타입을 가집니다. TypeScript 프로젝트에서는 타입 안정성과 코드 명확성을 높이기 위해 함수 매개변수에 명시적으로 타입을 지정하는 것이 좋습니다.
| const formatDate = (date) => { | |
| const formatDate = (date: number | Date) => { |
Rakhyunn
left a comment
Rakhyunn
left a comment
There was a problem hiding this comment.
오히려 UI가 화려하지 않아서 눈이 덜 피로하고 좋은데요?
그리고 필터 기능을 적용하여서 Todo를 관리할 수 있는 부분이 좋은 거 같습니다!
필터는 페이지 이동보다는 UI 상태 전환에 가깝다고 생각해서 TodoList에서 상태로 관리해서 리스트 목록만 바꾸는 방식이 페이지 이동도 잦지 않아 자연스러운 것 같은데 연수님 생각도 궁금합니다! 🧐
고생하셨습니다! 👍
|
할 일 목록/완료된 것/미완료된 것으로 탭을 나누어 두신 게 상당히 인상깊습니다! |
|
@Rakhyunn BrowserRouter 를 적용하는것 부터 해서 이후 여러 항목을 페이지로 분기하는것에 포커스를 두었는데 TodoList 앱에서는 리스트 목록만 바꾸는 방식으로 작업하는 방법이 더 효율적이고 좋은 방법 같습니다! 좋은 의견 감사합니다 ㅎㅎ |
|
기능적인 면에서는 조금 아쉬운게 데이터 넣고 중복 처리 된 게 아쉽네요 중복 처리 되는게 의도 한 것 일수 도 있지만 중복은 피해 주셨으면 하네요 |
|
@Sinou3936 네 ㅎ 고유 id 값이 api에서 가져온 값이랑 겹쳐서 데이터가 중복처리 되는걸 어떻게 처리해야할지 잘 모르겠습니다 조금 더 공부해보겠습니다 감사합니다 ㅎ |
|
페이지 분리를 진행중인 것과 완료된 것으로 분리한 것이 조금 더 다듬으면 실용성이 좋아보입니다 |
5 participants
TODOLIST
React, TypeScript 사용
TodoList.mov