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
[3기 전환오] TodoList with Ajax #18
Conversation
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.
안녕하세요 환오님! 몇 가지 리뷰 남겼습니다.
전체적으로 설계를 잘 해주신 것 같아요!
추가로 전역 상태관리를 담당하는 클래스를 만들어 보는건 어떨까요?
아 그리고 모든 메소드를 arrow function으로 사용하셨는데 혹시 특별한 이유가 있나요?
|
||
## 🕵️♂️ 제약사항 | ||
|
||
- [ ] 1. User의 이름은 최소 2글자 이상이어야 합니다. | ||
- [x] 1. User의 이름은 최소 2글자 이상이어야 합니다. |
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.
확인을 해봤는데 1글자 유저이름은 등록 안되는 걸로 보여요. 에러가 난 케이스를 설명해주실 수 있을까요?
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.
위의 요구사항은 user 생성 시에 구현해야하는 요구사항이라고 이해했어요. 하지만 할일 입력이 빈칸으로 입력됐을 때 undefined가 노출되는 점은 수정해야할 것 같네요. 감사합니다!
- [ ] 2. fetch api 사용하는 부분을 async await을 사용하여 리팩토링합니다. | ||
- [ ] 3. github issue에서 라벨을 붙이는 것처럼, 우선순위에 따라서 badge를 추가합니다. | ||
- [ ] 4. ES6 impot & export를 이용해 자바스크립트 파일을 리팩토링합니다. | ||
- [x] 1. 데이터를 불러오기전 로딩바를 이용해, 사용자가 데이터가 불러와지고 있다는 것을 보여줍니다. |
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.
저는 처음 todoitem 로딩 시에만 로딩바가 노출되도록 생각하고 구현해보았어요. (첫 로딩시, 유저생성 시, 유저삭제 시, 유저 변경 시 GET요청을 합니다)
준일님이 구현하신것을 보면서 생각해봤는데, 이게 이제 한 명 사용하는 앱이 아니다보니 다른 유저들이 수행한 작업을 언제 업데이트해야하는지 고민이 다시 드네요.
어느정도가 적정한 지점일지 고민을 한 번 해보아야겠습니다.
감사합니다!
this.activeUser = new State({}, this.render); | ||
this.filterType = new State(ALL, this.render); | ||
this.todoList = new ComputedState(this.computeTodoList, this.render, [ | ||
this.activeUser, | ||
this.filterType, | ||
]); |
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.
이 부분은 react의 useState와 유사하네요!
js/components/App.js
Outdated
const option = createFetchOption(POST, { contents }); | ||
const data = await fetch( | ||
`${API_BASE_URL}/api/users/${this.activeUser.value._id}/items/`, | ||
option | ||
); |
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.
fetch 관련 코드를 아예 분리시키는건 어떨까요?
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.
좀 더 추상화해도 좋을 것 같습니다. 감사합니다
js/components/App.js
Outdated
const data = await fetch( | ||
`${API_BASE_URL}/api/users/${this.activeUser.value._id}/items/`, | ||
option | ||
); | ||
this.activeUser.value = { | ||
...this.activeUser.value, | ||
todoList: this.activeUser.value.todoList.concat(await data.json()), | ||
}; | ||
}; |
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 data = await fetch( | |
`${API_BASE_URL}/api/users/${this.activeUser.value._id}/items/`, | |
option | |
); | |
this.activeUser.value = { | |
...this.activeUser.value, | |
todoList: this.activeUser.value.todoList.concat(await data.json()), | |
}; | |
}; | |
const { todoList, ...user } = this.activeUser.value; | |
const newItem = await fetch( | |
`${API_BASE_URL}/api/users/${user._id}/items/`, | |
option | |
).then(response => response.json()); | |
this.activeUser.value = { | |
...user, | |
todoList: [ ...todoList, newItem ], | |
}; | |
}; |
이렇게 표현할 수 있습니다!
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.
then을 활용하는게 읽기 더 깔끔한 것 같습니다!
js/core/State.js
Outdated
|
||
constructor(initialValue, render) { | ||
this.#value = initialValue; | ||
this.#renders = [render]; |
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.
Set으로 관리해보는건 어떨까요?
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.
Set이 더 적합한 구조인 것 같습니다!
@@ -0,0 +1,47 @@ | |||
export default class State { |
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.
State의 설계 방식은 굉장히 좋은 것 같아요!!
const createLoadingBar = () => | ||
` | ||
<li> | ||
<div class="view"> | ||
<label class="label"> | ||
<div class="animated-background"> | ||
<div class="skel-mask-container"> | ||
<div class="skel-mask"></div> | ||
</div> | ||
</div> | ||
</label> | ||
</div> | ||
</li> | ||
`; | ||
|
||
export default createLoadingBar; |
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.
이런 template이 컴포넌트에 들어있으면 어떨까요?
굳이 분리할 필요가 있을까요?
컴포넌트 클래스에 template이 존재할때의 장단점을 한번 비교해보면 좋을 것 같아요!
이러한 의견을 드리는 이유는 현대적인 프레임워크들은 대부분 템플릿이 컴포넌트에 포함되기 때문입니다.
반대로 서버 사이드에서 사용하는 MVC 방식의 경우 View가 아예 jsp 같은 형식으로 분리되어 있습니다.
왜 프런트엔드 프레임워크들은 템플릿(혹은 JSX)을 컴포넌트에 포함시키는걸까요?
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.
생각을 해보았는데
우선 분리를 했던 이유는 파일 하나 안에 존재하는 코드의 양을 줄이고 싶다는 이유였어요. 템플릿이 컴포넌트의 상태를 사용하기 때문에, 수정이 일어난다면 이 부분에 대한 신경을 계속 써줘야할 것 같긴합니다.
분리를 하지 않으면 컴포넌트에서 어떤 구조의 dom 템플릿을 갖는지 직관적으로 알 수 있을 것 같습니다. 또 컴포넌트의 상태가 어떻게 템플릿에서 사용되는지 바로 알 수 있다는 점이 장점일 것 같아요.
spa 프레임워크들이 어떤 이유에서 포함하는 건지 궁금하네요!
export const createFetchOption = (method, payload) => { | ||
const option = { | ||
method, | ||
headers: { | ||
'Content-Type': 'application/json', | ||
}, | ||
body: JSON.stringify(payload), | ||
}; | ||
|
||
if (!payload) option.body = ''; | ||
return option; | ||
}; |
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.
option만 추상화 하는게 아니라 아예 요청 자체를 추상화하면 더 좋을 것 같아요!
js/util/index.js
Outdated
@@ -0,0 +1,18 @@ | |||
export const convert2Html = (str) => { |
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.
stringToHtml
이런식으로 무엇을 무엇으로 바꾸는가를 명시하는게 더 좋을 것 같아요!
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.
맞는 말씀입니다 함수명을 좀 더 신경을 써서 지어야할 것 같습니다
저번에 알려주신 내용에 연장선 상으로, 코드컨벤션으로 화살표함수를 쓰는 것으로 정했다고 생각하고 코드를 짜봤어요.
이정도로 생각을 혼자 정리해보았는데 어떻게 생각하시는지 궁금합니다. |
절대 기분 상하거나 그러진 않아요! 다만 arrow function과 일반 function의 차이점이 분명히 존재하기 때문에 해당 사항만 인지하고 사용한다면 어떻게 사용해도 괜찮다고 생각합니다! |
표현식만 쓰면 호이스팅 때문애 제대로 생성자함수에서 멤버메서드가 불러와지지 않네요 ㅜㅜ |
보통 callback 함수내에서 사용되는 메소드의 경우에는 arrow function으로 해주시는게 좋습니다! |
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.
안녕하세요. 일이 너무바빠 접속도 못하다가 오늘 부랴부랴 코드리뷰남깁니다.
3주차도 잘부탁드립니다. 감사합니다!
addTodo = async (contents) => { | ||
const option = createFetchOption('POST', { contents }); | ||
const data = await fetch( | ||
`${API_BASE_URL}/api/users/${this.activeUser.value._id}/items/`, |
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.
fetch 부분을 따로 공통 모듈화시켜서 쓰면 더 좋지 않을까요?
그리고, Promise 오브젝트를 반환하는 fetch를 async await으로 사용하면 예외가 발생할 경우에 then().catch()를 사용하지 못하기에 try~catch문으로 래핑해주는게 맞을 것 같습니다.
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.
감사합니다! 모듈화를 진행할 때 같이 진행해보도록 하겠습니다!
|
||
initEventListener = () => { | ||
this.$target.addEventListener('keyup', ({ key, target }) => { | ||
if (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.
공백값에 대한 유효성도 추가되면 좋을 것 같습니다!
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.
if (!target.value) return
이 라인에서 공백값에 대한 유효성을 검사한다고 생각했습니다
$userTitle: $target.querySelector('#user-title'), | ||
$userList: $target.querySelector('#user-list'), |
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.
querySelector와 getElementId는 서로 트레이드 오프(범용성 및 응용범위와 속도)가있고 필요에 따라 사용을 하면된다고하는데, 이렇게 ID가 명확하게 나와있는경우에는 그냥 getElementId를 사용하는게 낫지 않을까요?
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.
getElementById가 queryselector에 비해 상대적으로 빠르기는 하나, 프로젝트 내에서 사용하는 함수의 횟수 querySelector의 성능(대략 초당 700만건 작업처리 가능하다네요)과 코드의 가독성을 고려해봤을 때 섞어서 사용하는 것 보다 통일하는 것이 낫다고 생각했습니다!
https://hashcode.co.kr/questions/5692/%EA%B0%95%EC%9D%98-4-11-queryselector%EC%97%90-%EC%84%B1%EB%8A%A5%EB%AC%B8%EC%A0%9C%EC%97%90-%EB%8C%80%ED%95%B4-%EC%A7%88%EB%AC%B8-%EB%93%9C%EB%A6%BD%EB%8B%88%EB%8B%A4
this.$target.addEventListener('click', (e) => { | ||
e.preventDefault(); | ||
const { target } = e; |
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를 구조분해하기보다는 eventListener부분에서 콜백이올 때 바로 ({ target})으로 구조분해하면 좋지 않을까요?
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.
앵커태그의 기본동작을 방지하려는 의도로 preventDefault함수를 사용하려 이벤트객체를 직접 받아 보았습니다.
다만 예외에 대한 처리는 다른 방법으로 이루어서 빼도 될 것 같아요
case 'FIRST': | ||
case '1': | ||
return ` | ||
<select class="chip select primary"> | ||
<option value="1" selected >1순위</option> | ||
<option value="2">2순위</option> | ||
<option value="0">미지정</option> | ||
</select> | ||
`; | ||
|
||
case 'SECOND': | ||
case '2': | ||
return ` | ||
<select class="chip select secondary"> | ||
<option value="2" selected>2순위</option> | ||
<option value="1">1순위</option> | ||
<option value="0">미지정</option> | ||
</select> | ||
`; | ||
|
||
default: | ||
return ` | ||
<select class="chip select "> | ||
<option value="0" selected >순위</option> | ||
<option value="1">1순위</option> | ||
<option value="2">2순위</option> | ||
</select> | ||
`; | ||
} | ||
}; |
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문의 or조건 사용해서 구현하신거 좋은생각인거같네요!
데모
https://jho2301.github.io/js-todo-list-step2/
🎯 요구사항
🎯🎯 심화 요구사항
🕵️♂️ 제약사항
주안점
+@
할 일
추가로 하면 좋을 아이디어