-
Notifications
You must be signed in to change notification settings - Fork 11
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
[4주차] 김현민 미션 제출합니다. #14
base: master
Are you sure you want to change the base?
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.
멋진 코드였습니다. 코드 리뷰에 감탄 밖에 없어서 죄송하네요. 저에게 리뷰 달아주신 것 봤는데, 도움이 많이 되었어요. 멋진 코드를 통하여 제가 어떻게 발전할 수 있는지 알 수 있었습니다. 또한, 궁금했던 내용들이 많이 해결되었어요. 고생하셨습니다. 멋져요.정말
ref.current.scrollTop = scrollHeight - clientHeight; | ||
} | ||
}; | ||
|
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.
커스텀 훅에대한 사용방식과 왜 사용해야 하는지에 대해 고민하고 있었는데, 운이 좋게 코드를 볼 수 있었네요 . 많이 배워가요~~~~~
message, | ||
icon, | ||
onClick, | ||
}: Omit<GroupChatRoomState, "id" | "people" | "type">) { |
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.
omit에 대한건 생각하지 않고있었는데 유용하게 쓰시는 것 배워갑니다.........
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.
저도 typescript가 아직 생소해서 배워갑니다~
"type", | ||
CHATROOM_TYPE.MAIN | ||
); | ||
const searchedSubGroupChat = filterByCategory( |
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.
filterbycategory가 뭔지 봤는데, 따로 빼서 정의해서 사용하시는 것 보고 감탄했습니다 배워가요
@@ -0,0 +1,24 @@ | |||
import { useState } from "react"; | |||
import { ChatDetailState } from "../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.
커스텀 훅을 적절하게 잘 만들어 쓰시는 것이 역시 대단하시군요
chatData?: ChatDetailState[] | []; | ||
setChatData?: React.Dispatch<React.SetStateAction<ChatDetailState[] | []>>; | ||
setShouldScrollToBottom?: React.Dispatch<React.SetStateAction<boolean>>; | ||
} |
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.
이런 detail한 상태 정의를 따로 하신 것 보고 역시 경력이 많으시고, 잘하시는구나를 느꼈습니다. 배워갑니다!
${(props) => props.theme.fontStyles.body2} | ||
} | ||
`; | ||
|
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)에선 전윤수씨와만 대화할 수 있어서 조금 아쉬웠지만... 사실 그 기능까지 하면 너무 힘들거같아요ㅎㅎㅎ 코드 성능까지 신경써서 깔끔하게 짜려고 노력하신 것들이랑 여러 방법들도 배워갑니다!!!
@@ -0,0 +1,14 @@ | |||
{ | |||
"compilerOptions": { | |||
"baseUrl": "./src", |
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.
말씀하신 것처럼 절대경로 설정하신 것 좋은 것 같습니다! 저도 다음에는 설정해야겠어요..!!
<Battery /> | ||
</span> | ||
</Status> | ||
</StatusBarWrapper> |
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 DividerWrapper = styled.div<DividerProps>` | ||
width: ${(props) => | ||
props.state === DIVIDER_TYPE.SHORT ? "33.5rem" : "37.3rem"}; |
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.
다음부터는 주요 기능에 대한 주석 꼭 달아가며 작업 하도록 하겠습니다!!
setChatData, | ||
}); | ||
const handleKeyPress = (event: React.KeyboardEvent<HTMLInputElement>) => { | ||
if (event.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.
이 부분에 어떤 분은 이런 현상이 없고 어떤 분은 이런 현상에 대해 말씀해주셔서 사용하는 기종에 따라 다른건지 한번 찾아봐야될 것 같아요. 감사합니다!!
const searchedFrontEndList = filterByCategory( | ||
searchedFriendsList, | ||
"majorIn", | ||
MAJOR_TYPE.FRONTEND |
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.
major_type을 enum으로 따로 나눠서 나누신 디테일함이 좋습니다!
{searchedDesignerList.length > 0 && searchedFrontEndList.length > 0 ? ( | ||
<Divider | ||
state={DIVIDER_TYPE.SHORT} | ||
$addClass={`background-color:${theme.colors.gray5}`} |
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.
리뷰에 써주셨던 것처럼 다음에는 저도 theme 이용해보도록 하겠습니다..!
<Time> | ||
{doubleClicked ? ( | ||
<LikeIcon $isUser={isUser}> | ||
<Like /> |
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.
double click 하면 하트 나오는 디테일 좋습니다! bubble component로 따로 빼서 깔끔하게 짜려고 신경쓰신 것 같아요!
message, | ||
icon, | ||
onClick, | ||
}: Omit<GroupChatRoomState, "id" | "people" | "type">) { |
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.
저도 typescript가 아직 생소해서 배워갑니다~
배포 링크
주요 기능 설명
ceos.1.mp4
ceos.2.mp4
ceos.3.mp4
ceos3.mp4
ceos.1.mp4
ceos.2.mp4
ceos.1.mp4
ceos.2.mp4
고민 과정
이전 과제와 달리 라우팅이 추가되고 페이지가 늘어난 만큼 폴더 및 파일 구조와 효율적인 코드 작성에 대해 많은 고민을 하며 과제를 진행하였습니다.
폴더 및 파일
혼자만 보는 코드가 아니라 다른 팀원과 협업을 한다고 생각을 하고 폴더와 파일들을 나름의 기준을 갖고 정리해 보았습니다. 여러 파일에서 쓰는 component, state, constant 등은 common 폴더에, user,chat,friend 라는 특성을 feature 폴더에 정리하여 각각의 파일을 정리하였습니다. 또한 라우팅되는 각각의 페이지들은 page 폴더에, style 과 관련된 폴더는 style 폴더에 정리하였습니다. 이처럼 중간에 코드를 보는 사람이 최대한 쉽게 구조를 파악할 수 있도록 고민을 하며 짜보았습니다.
제네릭 함수
현재 홈 화면에서 검색을 통해 그룹 채팅 리스트를 필터링 하는 기능과 친구 목록 페이지에서 친구 리스트를 검색을 통해 필터링 하는 기능이 구현되어 있습니다. 그러나 같은 검색 기능을 2개의 메소드로 나누어 실행을 한다면 비효율적인 코드가 될 것입니다. 이로 인해 제네릭 함수를 사용하게 되었습니다. 서로 다른 두 state를 받은 후 받은 state 의 공통요소인 name 에 접근하여 필터링 후, 처음에 받았던 state 의 리스트를 반환형으로 선언해주면 하나의 함수로 현재 구현된 2개의 기능은 물론, 추후에 다른 검색 기능도 이 함수를 활용하여 작성할 수 있으므로 확장성이 좋은 코드인 것 같습니다.
검색 기능 밖에도 채팅 타입과 유저의 전공을 가지고 각각의 데이터를 필터링할 수 있는 제네릭 함수를 사용하였습니다.
보기 싫은 import 문
상대경로와 export 들을 각각 import 해온다면 import 가 매우 많아질 것이고 코드를 볼 때 어지럽기 까지 합니다. 이를 방지하기 위해 craco 를 통한 절대경로 및 index.ts 파일들을 통해 한꺼번에 import 를 할 수 있게 설정해 보았습니다.
전역 상태 관리에 대한 고민
모든 기능들을 사용할 떄 이 기능을 꼭 써야만 하는 이유가 가장 중요하다고 생각했습니다. 사실 지금은 많은 기능이 없기 때문에 많은 고민 끝에 현재 파일에서 전역적으로 가져다 쓰고 수정할 정보가 없다고 판단하였습니다. 물론 억지로 도입을 할 수도 있었겠지만 현재 전역 상태 관리를 쓸 이유를 개인적으로 찾지 못했기 때문에 이번 과제에서는 도입을 하지 않았습니다.
배포
나중에 진행할 프로젝트에서는 도커와 aws 를 이용한 배포 과정은 필수적이라고 생각했습니다. 전에 배포 경험이 있었긴 했지만 꽤 오랜시간 건드리지 않아 까먹는 일이 없도록 하기 위해 프리티어 ec2 와 도커를 이용하여 배포해 보았습니다.
해결되지 않은 챌린지
아직 의미적으로 완전치 않은 코드가 존재합니다. 현재 채팅 말풍선 상태에 선택적이긴 하지만 해당 채팅 말풍선이 포함된 채팅 방의 데이터 props 가 선언되어 있습니다. 더블클릭 시 doubleClicked 상태를 바꾸어 하트 표시를 구현해야 했고, 이를 위해 전체 데이터에서 해당 데이터를 찾아 상태를 변경해야 했기 때문입니다. 그러나 아무리 선택적이라고 해도 각각의 말풍선 state 에 채팅 데이터가 들어있는 것이 의미적으로 말이 안된다고 생각했습니다. 또 전역적으로 atomFamily 와 selectorFamily 를 활용할까 고민도 해봤는데 채팅 데이터는 채팅 페이지에서만 사용하는데 이를 전역적으로 선언해버리는 것은 오버코딩이라고 판단했습니다. 우선 시간상 제출을 하였는데 추후에 데이터를 props 로 넘기는 것이 아닌 말풍선 컴포넌트에서 로컬스토리지에 현재 저장되어 있는 데이터를 직접 부른 후 변경하는 방식으로 리팩토링을 진행할 예정입니다.
정리
SPA의 개념 이해
react-router-dom 라이브러리를 활용하여 넘어오는 하나의 index.html 파일에서 설정한 라우트에 따라 분기처리 하여 각각 알맞은 페이지를 띄워주었습니다.
디자이너QA
1-a
1-b
1-c
2-a
2-b
2-c
2-d
이 밖에도 많은 QA 들을 진행하였고, 매우 꼼꼼히 봐주시고 피드백 해주셔서 수월하게 작업을 마칠 수 있었습니다. :)