This repository was archived by the owner on Sep 19, 2021. It is now read-only.
[2주차] 최승원 미션 제출합니다.#4
Open
seungwon2 wants to merge 15 commits into
Open
Conversation
ybh1760
suggested changes
Apr 11, 2020
Comment on lines
+24
to
+27
| border-width: initial; | ||
| border-style: none; | ||
| border-color: initial; | ||
| border-image: initial; |
There was a problem hiding this comment.
border-style속성의 기본 값이 none이라서 생략해주셔도 됩니다.
border-width, border-color, border-image 속성 값을 initial로 하면, 기본 값으로 설정하는 거기 때문에 없어도 되는 부분입니다!
Comment on lines
+38
to
+41
| border-width: 1px; | ||
| border-style: solid; | ||
| border-color: rgb(97, 97, 97); | ||
| border-image: initial; |
There was a problem hiding this comment.
이 부분은 아래와 같이 border속성을 통해서 한번에 정리할 수 있습니다.
Suggested change
| border-width: 1px; | |
| border-style: solid; | |
| border-color: rgb(97, 97, 97); | |
| border-image: initial; | |
| border:1px solid rgb(97, 97, 97); |
Comment on lines
+11
to
+12
| const [date, setDate] = useState(); | ||
| const [task, setTask] = useState(); |
There was a problem hiding this comment.
date와 task state는 입력받을 때만 사용되고 입력이 끝나면 리셋되는 state인 것 같네요. 실제로 사용되는 부분 외에서 state를 정의하여 사용하다 보면 불필요한 리렌더가 발생할 수 있어서, date와 task는 todo-input.js에서 state관리를 해주는 게 좋을 것 같아요.
Comment on lines
+51
to
+55
| const onClickComplete = (id) => { | ||
| const newTodos = [...todos]; | ||
| setTodoList(newTodos.filter((todo) => todo.id !== id)); | ||
| console.log(newTodos); | ||
| }; |
There was a problem hiding this comment.
완료 버튼 누르면 삭제하는 함수인 거 같네요. 이 함수는 todo-item.js에서 정의해서 사용하면 좋을 거 같아요!
Comment on lines
+31
to
+48
| if (dateFlag === 0 || taskFlag === 0) { | ||
| alert("모든 항목을 입력해주세요!"); | ||
| } else { | ||
| if (date.length === 8 && date > 20200101) { | ||
| setTodoList([ | ||
| ...todos, | ||
| { | ||
| id: todos.length, | ||
| date, | ||
| task, | ||
| }, | ||
| ]); | ||
| stateReset(e); | ||
| alert("입력 완료!"); | ||
| } else { | ||
| alert("날짜를 올바른 형식으로 입력해주세요!"); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분도 잘 해주셨는데, 몇 가지 개선할 점 정리해드릴게요!
※ 아래 말을 생각하시면서 읽으면 좋을 것 같아요~
- date와 task를 todo-input.js에서 관리하신다면, validation하는 것도 모두 todo-input.js에서 해주셔야합니다!
- addTodo 함수에서 입력 유효성을 같이 체크하고 있는데 이렇게 하게 되면 유효성 체크하는 로직을 한 눈에 보기 어려운 거 같아요. 그래서 validateTodo 함수를 따로 선언해서 유효성체크를 해주시면 좋을 거 같아요.(아래처럼요)
const validateTodo = () =>{
let isValid=false;
....(checking date, todo validation)
return isValid;
}
const addTodo = () => {
if(!validateTodo()) return;
setTodoList([
...todos,
{
id: todos.length,
date,
task,
},
])
}
- dateFlag와 taskFlag를 선언해서 validation을 해주셨는데 이렇게 하게 되면 허점이 생길 수 있습니다. onChange에서 입력을 받다가 입력을 모두 지운경우에 taskFlag는 여전히 onChange가 호출된 횟수를 갖고 있기 때문에 아무런 내용이 없어도 첫번째 validation을 통과하게 됩니다. 그래서 이 부분은 task.length를 이용해서 validation하면 좋을 거 같아요.
아래 코드 참조해주시면 이해하기 편하실 거 같아요!
isDateValid= (date.length === 8 && date > 20200101);
isTaskValid= task.length > 0;
if(!isDateValid){
alert("날짜를 올바른 형식으로 입력해주세요!");
}
if(!isTaskValid){
alert("할 일을 입력해주세요!");
}
return isDateValid&&isTaskValid;
- todos의 id 값으로 todos.length로 하셨는데 이렇게 되면 중간에 삭제하고 난 후 새로운 아이템을 추가할 때 중복된 id를 갖는 todo가 생길 수 있어요. 그래서 id로 사용하기 적합하지 않은 것 같네요. task는 중복 될 일이 거의 없어서 task.toString()을 쓰면 좋을 거 같아요~
greatSumini
reviewed
Apr 15, 2020
|
|
||
| const validateTodo = () => { | ||
| let isValid = false; | ||
| let isDateValid = date.length === 8 && date > 20200101; |
Member
There was a problem hiding this comment.
Suggested change
| let isDateValid = date.length === 8 && date > 20200101; | |
| const isDateValid = date.length > 8 && date > 20200101; |
- 오타가 있네요 😅
- 값에 변화가 없어서 const를 써도될 것 같아요~
Comment on lines
+34
to
+35
| isValid = isDateValid && isTaskValid; | ||
| return isValid; |
Member
There was a problem hiding this comment.
Suggested change
| isValid = isDateValid && isTaskValid; | |
| return isValid; | |
| return isDateValid && isTaskValid; |
불필요힌 코드는 합쳐주세용~
| setTodoList([ | ||
| ...todos, | ||
| { | ||
| id: todos.length, |
Member
There was a problem hiding this comment.
이 id는 unique key 역할을 할 수 없어요!
[1, 2, 3]
2 완료
[1, 3]
todo 추가
[1, 3, 3]
이렇게 됩니다! ㅠ id는 다른 방식으로 구현해야할 것 같아요
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hooks와 함수형으로 구조를 짜는 것이 익숙하지 않아서 헤멘 부분이 있었습니다. 확실히 저번 미션보다 시간도 더 걸렸는데 그만큼 props와 state에 대해서 더 공부할 수 있었던 것 같습니다. 다음번에는 함수를 어떤 컴포넌트에 위치하면 좋을지에 대해서도 더 고민해보고 싶습니다.
https://react-todo-11th-lmr1jr4cw.now.sh/