-
Notifications
You must be signed in to change notification settings - Fork 4
[mission002] TODOS 시스템에 비동기/온오프라인을 고려하여 업데이트하기 #14
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
base: master
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| onKeyDown = async (e) => { | ||
| if (e.key === ENTER) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.key를 썼을 때, input value가 한글일 경우 리스트에 두 개씩 추가되는 버그가.. 있더라구요.
이건 e.key 대신 e.keyCode를 쓰면 해결되는데, keyCode가 표준이 아니라서ㅜㅜ..
다른 분들은 어찌하셨는지 궁금하네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동준님이 해결해주셧녜요 ㅠㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 e.code일때 두번 입력되어서 e.key로 변경하고 작동도 정상이라 해결했다고 생각했는데 e.key도 동일한 증상이 있나요?
@StellaKim1230 혹시 동준님이 어떻게 해결하셨는지 알려주실 수 있나요??
e.key를 썼을 때, input value가 한글일 경우 리스트에 두 개씩 추가되는 버그가.. 있더라구요.
이건 e.key 대신 e.keyCode를 쓰면 해결되는데, keyCode가 표준이 아니라서ㅜㅜ..
다른 분들은 어찌하셨는지 궁금하네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StellaKim1230 동준님 PR #15 리뷰내용 중 엔터 연속입력시 처리방법 말씀하시는 거였군요. e.key랑은 다른 문제였네요. 😓
| @@ -0,0 +1,22 @@ | |||
| const username = 'kimjieun' | |||
|
|
|||
| export const apiHandler = async ({ url, method, body, customUrl }) => { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apiHander 함수를 만들어서 하나로 쓰는 게 인상깊습니다.
중복코드가 많이 줄어들 것 같아요.
| @@ -0,0 +1 @@ | |||
| export const hostUrl = 'http://todo-api.roto.codes' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hostUrl만 따로 url.js로 관리하시는 이유가 있을까요?
username은 api.js에서 관리하고, hostUrl은 url.js에서 관리하는 기준이 궁금합니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 실수한 부분입니다ㅎㅎ 따로 관리하는게 맞을것 같아요 ㅎㅎ
| } | ||
|
|
||
| onKeyDown = async (e) => { | ||
| if (e.key === ENTER) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enter 키를 눌렀을 때 -> input value가 비어있으면, 값이 추가되지 않는 조건을 넣어주면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오! 꼼꼼한 리뷰 감사합니다!
mission002/kimjieun/js/App.js
Outdated
| } | ||
| } | ||
|
|
||
| this.fetchTodoData() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input에 값을 칠 때마다 get api가 불러와지네요.
Enter키를 눌러 값이 추가되었을 때만 불러와도 될 것 같습니다.
s280493
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
올려주신 궁금한 점 1, 2, 3이 제 컴퓨터에서는 재현이 안 되더라구요!
아무 콘솔 오류도 없어요.
지은님 컴퓨터에서는 어떻게 보이는지 궁금하네요~
| this.fetchTodoData() | ||
| } | ||
|
|
||
| onTodoCheck = (todoStatus) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해야할 일, 완료한 일 필터를 눌렀을 때 필터클래스에 selected를 넣어서 필터주위에 빨간 테두리가 유지되어있으면 더 좋을 것 같아요.
mission002/kimjieun/index.html
Outdated
| <link rel="stylesheet" href="css/style.css" /> | ||
| </head> | ||
| <body> | ||
| <div id="app"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div id="app">를 <div id="app" class="todoapp">로 변경하면 기존 css가 잘 적용될 것 같아요.
YongHoonJJo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 늦게 드려 죄송합니다.ㅜ 눈에 보이는 몇 가지에 대해 리뷰 남겨보았습니다..!!
그리고, 질문 1, 2, 3은 해결 되신건가요?
1, 2 는 this 에 해당 프로퍼티들이 없어서 발생한 문제 같아 보여서요..ㅜ
| this.$selector.addEventListener('click', (e) => { | ||
| if (e.target.className === ACTIVE) this.onTodoCheck(ACTIVE) | ||
| if (e.target.className === COMPLETED) this.onTodoCheck(COMPLETED) | ||
| if (e.target.className === ALLSELECTED) this.onTodoCheck(ALLSELECTED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 만족하는 조건식이 있어도 나머지 조건식들에 대한 검사가 이루어질 것 같아요!! switch-case 문이나, this.func() 실행 후 return 을 통해 함수를 확실히 종료해주는 것은 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
용훈님 말씀대로 return 이나 swtich case 문으로 바꿔 주는 것이 좋겠네요! 필요없는 로직 검사가 안이루어지니깐 ㅎ 조언 감사합니다
| init = () => { | ||
| this.$selector.addEventListener('click', (e) => { | ||
| if (e.target.className === DESTROY) this.onDeleteTodo(e.target.parentNode.dataset.idx) | ||
| if (e.target.className === TOGGLE) this.onToggleTodo(e.target.parentNode.dataset.idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const className = e.target.className 과 같이 프로퍼티 캐싱처리 후 위에서 언급했던 것 처럼 switch-case 문이나 if 문 뒤에 return 으로 종료시켜주는 것은 어떨까요?
|
|
||
| createLiClassName = (isCompleted) => { | ||
| if (!isCompleted) return 'view' | ||
| if (isCompleted) return 'completed' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isCompleted 의 경우 T/F 만 존재하기 때문에,
if (!isCompleted) return 'view'
return 'completed'
와 같이 하면, 굳이 return '' 를 넣지 않아도 될거 같아요.
그리고 return isCompleted ? 'completed' : 'view' 와 같이 처리할 수도 있구요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇네요! 항상 삼항연산자는 다른사람이 리뷰해주면 생각나는듯 ㅠㅠ 습관화시켜야겠어요 ㅠ
| createTodoListHtmlString = ({ content, isCompleted, _id }) => { | ||
| return `<li class=${this.createLiClassName(isCompleted)}> | ||
| <div data-idx=${_id} class="view"> | ||
| <input class="toggle" type="checkbox" ${isCompleted === true && 'checked'}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isCompoled 가 false 면 <input class="toggle" type="checkbox" false> 이런식으로 남게 되지 않을까요? (전에 제가 이렇게 했었지만요..ㅎ)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅎㅎ 테스트해보겠습니다! ㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false 안남는것으로 확인했습니다 ㅎㅎ
| const todoData = this.data.filter(d => { | ||
| if (todoStatus === ACTIVE) return !d.isCompleted | ||
| if (todoStatus === COMPLETED) return d.isCompleted | ||
| if (todoStatus === ALLSELECTED) return this.data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.data 는 항상 true 를 리턴해야하기 때문에 이렇게 두신건가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체 할일 선택시에는 모든 데이터가 나와야 하기 때문에 저렇게 구현 한 것 같아요!
제가 new App()을 안했었던 것 같기도 허고 ㅠㅠ 다시 해봐야겠네요 ㅠㅠ |
아뇨 아직 ㅠ this에 해당 프로퍼티 들이 없다는 말씀은 this.setState() 이 함수자체가 없다는 말씀이신가요??ㅠㅠ |
new App() 을 안하면 |
#7 [mission002]
궁금한점.
코드 시도 후 아래와 같은 에러가 나왔는데, 이유를 모르겠습니다. 같이 고민해보아요!