-
Notifications
You must be signed in to change notification settings - Fork 8
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
채팅방 버그 이슈 및 프로필 사진 변경 모달 UI 수정 #139
Conversation
useEffect(() => { | ||
const selectedUserIds = selectedUsers.map((user: any) => user.userDocumentId); | ||
setFilteredUserList(allUserList.filter((user: any) => selectedUserIds.indexOf(user._id) === -1)); | ||
}, [selectedUsers]); | ||
const [selectedUsers, filteredUserList, searchUser, deleteUser, addSelectedUser] = useSelectUser(inputBarRef); | ||
|
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.
커스텀훅을 사용해서 view의 역할에 더 알맞게 코드가 바뀐 것 같아서 아주 좋네요!
|
||
let instance: any = null; | ||
class UserService { | ||
constructor() { | ||
if (instance) return instance; | ||
instance = this; | ||
} | ||
|
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.
activity 부분을 따로 서비스로직을 빼주셨군요! class 명을 ActivityService라고 해야할 것 같아요
async getActivityList(userDocumentId: string, count: number) { | ||
const user = await Users.findById(userDocumentId, ['activity']); | ||
const newActivityList = await Promise.all(user!.activity.slice(count, count + 10).map(async (activity: IActivity) => { | ||
const detailFrom = await this.findUserByDocumentId(activity.from); |
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.findUserByDocumentId
여기서 this는 UserService
의 메서드라서 아마 수정이 필요하지 않을까요?
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.
Users.findById
로 바꾸면 될 것 같습니다
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.
수고하셨습니다! 코드가 깔끔해졌어요..!
-
userService 부분의 로직을 분리하는게 훨씬 가독성이 좋은 것 같아요! 처음에 저희가 싱글톤 클래스를 적용을 한 이유가 메서드에서
this
를 사용하기 위함이었는데 그게 필요가 없도록 분리할 수 있다면 기능별로 나누어도 될 것 같습니다. 그리고 typescript에서 싱글톤과 네임스페이스 문법을 지원하는 것 같아서 찾아봤습니다 !
https://radlohead.gitbook.io/typescript-deep-dive/main-1/singleton -
컴포넌트 폴더 구조는 감이 잘 안오네요.. 개인적으론 api는 어느 컴포넌트에서 쓰이냐보다 어떤 api를 호출하는지가 중요한 것 같아서 api 폴더에 api 라우터 네임별로 정리하는게 어떨가 생각하는데 어떠신가요?
-
컴포넌트는 적어주신대로 도메인 별 view 파일들과 아닌 파일들을 분리하고 재사용이 되는 style들을 분리하는 형태로 하면 좋을 것 같아요 이 부분은 의견 들어보면서 다른 프로젝트 구조들도 더 참고해보겠습니다!
개요
변경 로직
변경후
채팅 방 버그
2021-11-26.12.25.40.mov
프로필 사진 변경 UI 수정
2021-11-26.12.27.17.mov
기타