[코드리뷰 요청] 과제2 완료 #170
[코드리뷰 요청] 과제2 완료 #170
Conversation
src/App.jsx
Outdated
| import TodoLists from './components/TodoLists'; | ||
|
|
||
| export default function App() { | ||
| const [inputText, setInputText] = useState(''); |
There was a problem hiding this comment.
input이라는 이름이 붙은 이유가 <input />태그로 쓰이거나 혹은 사용자의 입력으로 사용되는 텍스트라서 그런 것 같네요. 만약 이러한 텍스트가 LocalStorage나 외부 API를 통해서 값이 변경될 수 있다면 그때는 이름을 변경해야겠네요
src/App.jsx
Outdated
| export default function App() { | ||
| const [inputText, setInputText] = useState(''); | ||
| const [todos, setTodos] = useState([]); | ||
| const randomId = () => Math.random().toString(12).substr(2, 16); |
There was a problem hiding this comment.
아이디를 만들기 위해서 랜덤한 값으로 만들고 계시군요. 만약에 아이디가 겹치지 않도록 하려면 UUID를 사용하거나 nanoid같은 것을 사용하는 것이 좋겠어요. 혹은 값을 1부터 계속 증가시켜서 겹치지 않도록 할 수 있습니다.
src/App.jsx
Outdated
| return ( | ||
| <div> | ||
| <Header /> | ||
| <TodoInput |
There was a problem hiding this comment.
이 컴포넌트 이름이 <TodoForm>이 되는 것은 어떨까요?
src/components/TodoLists.jsx
Outdated
| const showTodos = () => { | ||
| if (todos.length === 0) { | ||
| return <span>할 일이 없어요!</span>; | ||
| } | ||
| return todos.map((item) => ( | ||
| <li key={item.id}> | ||
| <span>{item.text}</span> | ||
| <button type="button" id={item.id} onClick={onClickComplete}> | ||
| 완료 | ||
| </button> | ||
| </li> | ||
| )); | ||
| }; |
There was a problem hiding this comment.
이부분이 복잡하다고 생각하셔서 함수를 만드신 것 같습니다. 컴포넌트로 분리하는게 더 좋아보이네요.
|
피드백 반영해봤습니다!ㅎㅎ
|
src/App.jsx
Outdated
| import TodoLists from './components/TodoLists'; | ||
|
|
||
| export default function App() { | ||
| const [newTodo, setNewTodo] = useState(''); |
There was a problem hiding this comment.
새롭게 투두로 등록할거라서 newTodo인 것 같아요. 나중에 수정 기능이 생기면 이 값으로 똑같이 사용할 것 같은데 그러면 이 변수의 이름은 newOrUpdateTodo가 되어야만 할거예요.
src/components/TodoLists.jsx
Outdated
| export default function TodoLists({ hasTodos, todos, onClickComplete }) { | ||
| return ( | ||
| <> | ||
| {hasTodos ? ( | ||
| <ShowTodos todos={todos} onClickComplete={onClickComplete} /> | ||
| ) : ( | ||
| <NoneTodos /> | ||
| )} | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
제가 의도했던 건
<li key={id}>
<span>{text}</span>
<button type="button" id={id} onClick={() => onClickComplete(id)}>
완료
</button>
</li>이 부분을 빼는 것을 생각했었어요 ㅎㅎ 여기 코드는 3항 연산자도 좋지만 Guard clause로 작성하는 것도 좋을 것 같아요.
| export default function TodoLists({ hasTodos, todos, onClickComplete }) { | |
| return ( | |
| <> | |
| {hasTodos ? ( | |
| <ShowTodos todos={todos} onClickComplete={onClickComplete} /> | |
| ) : ( | |
| <NoneTodos /> | |
| )} | |
| </> | |
| ); | |
| } | |
| const isEmpty = (arr = []) => arr.length === 0; | |
| export default function TodoLists({ hasTodos, todos, onClickComplete }) { | |
| if (isEmpty(todos)) { | |
| return <NoneTodos />; | |
| } | |
| return ( | |
| <ShowTodos todos={todos} onClickComplete={onClickComplete} /> | |
| ); | |
| } |
See also
There was a problem hiding this comment.
헉 코드 정말 깔끔해졌네요!
저는 hasTodos라는 이름으로 todo의 유무도 컴포넌트 인자로 넘겨주면서 너무 투머치 한 느낌도 있었거든요
isEmpty라는 함수로 빼서 저렇게 사용하는 것도 좋네요!
그리고 처음에 짤 때 가드클래스를 사용했었는데 윤석님이 짜신 것처럼 바로 export default에 넣을 생각을 못했었는데.. 익숙치 않아서 그랬던 것 같습니다!
보내주신 코드에서 지향한 부분 앞으로 코드에도 잘 녹여서 진행해볼게요! 감사해요 :)
2 participants
과제 2 완료했습니다! 코드리뷰 요청드려요 :)