-
Notifications
You must be signed in to change notification settings - Fork 8
[5주차] 고윤정 미션 제출합니다. #17
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: main
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.
오늘도 다시 한 번 윤정님의 뛰어난 디자인 감각과 섬세함을 다시 느끼고 갑니다 ㅎㅎ 과제하시느라 고생 많으셨습니다 ! 자동 리다이렉트까지 정말 디테일함에 놀랐습니다 ㅎㅎ 리뷰 확인 부탁드려요 ~
| [chatRoomId]: [...(prevMessages[chatRoomId] || []), newMessage], | ||
| }; | ||
| }); // 기존 메시지에 새로운 메시지 추가 | ||
| } |
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.
위 코드를 App에서 작성하신 이유가 있을까요 ? InputBox 안에서만 사용하는 코드인 것 같은데, InputBox 컴포넌트 내에서 선언 및 사용하시는게 좋을 것 같습니다. 불필요하게 props를 내려주고 있는 것 같아요 ~
| const root = createRoot(container).render( | ||
| <RecoilRoot> | ||
| <BrowserRouter> | ||
| {/* <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.
사용하지 않는 코드는 지워주세용~
| time: '오후 09:01', | ||
| }, | ||
| { | ||
| id: 10, |
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.
메시지의 id 값 준 것 너무 좋네요
| <div css={contentStyle}> | ||
| {filteredMessages.map((msg) => { | ||
| const user = getUserInfo(msg.userId); // 메시지 작성자 | ||
| const isUser = msg.userId === 0; // 사용자 유저아이디 : 0 |
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.
전반적으로 메시지가 내꺼인지, 상대방의 것인지 구분하는 코드에서 userId가 0인지 아닌지 비교하는 코드를 사용하시는 것 같습니다.
이렇게 코드의 다양한 곳에서 0과 비교하게 되면 유지보수 측면에서 불리할 수 있습니다 (만일 내 userId를 1로 바꾸어야 한다면 모든 코드에 있는 0을 1로 바꾸어 주어야겠죠 ? )
이럴때는 사용자 유저 아이디를 상수화 하는게 좋을 것 같습니다 ! 매직넘버, 매직리터럴은 지양하는 게 좋아요 ~
ongheong
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.
안녕하세요, 윤정님의 5주차 미션 코드 리뷰를 맡게 된 심여은이라고 합니다!
이번에 저처럼 처음 전역 상태 관리 라이브러리를 적용해보신 것 같은데, atom에 상태들을 잘 정리하고, useRecoilValue와 useRecoilState를 적절하게 사용해보신 것 같아 매우 좋습니다.
항상 좋은 결과물을 내주셔서 프로젝트때마다 기대하고 있습니다! 이번 프로젝트하느라 정말 많으셨고, 다음 프로젝트도 많이 기대하겠습니다! 👍🏻
| <div css={contentStyle}> | ||
| {filteredMessages.map((msg) => { | ||
| const user = getUserInfo(msg.userId); // 메시지 작성자 | ||
| const isUser = msg.userId === 0; // 사용자 유저아이디 : 0 |
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.
저도 이번 프로젝트에서 이러한 식으로 많이 작성했는데, 확실히 상수화 해서 사용하는게 좋을 것 같네요! 많이 배워갑니다😅
| <Route path="/" element={<Navigate to="/list" />} /> | ||
| <Route path="/list" element={<List />} /> | ||
| <Route path="/chatroom/:chatId" element={<App />} /> | ||
| <Route path="*" element={<Navigate to="/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.
*로 path를 한번 더 설정한 이유가 있을까요?
| userMessages.length > 0 | ||
| ? userMessages[userMessages.length - 1].text | ||
| : '아직 메시지가 없습니다.'; |
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.
마지막 메시지가 없을 때 이런 꼼꼼한 처리 좋아요!🙂
| // Do Not Disturb 상태 정의 | ||
| export const doNotDisturbState = atom<boolean>({ | ||
| key: 'doNotDisturbState', | ||
| default: 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.
방해금지모드의 boolean 변수 상태 관리는 각 채팅방마다 다르게 적용되어야 하니까, 해당 컴포넌트에서 지역으로 관리하는게 좋을 것 같아요!
| { | ||
| "routes": [{ "src": "/[^.]+", "dest": "/", "status": 200 }] | ||
| } |
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.
이 부분은 vercel로 배포할 때 주어야 하는 설정인가요? 각각 어떤 의미를 갖고 있는지 궁금해요!
안녕하세요! LG U+ 유레카 프론트엔드 1기 고윤정입니다.
이번 주차는 새로 시도해보는 것들이 많아 검색을 정말 많이 한 것 같습니다.. 하지만 그만큼 배워간 것도 많아서 좋습니다!
상태관리라는 것을 처음 해봤는데 왜 상태관리 라이브러리를 사용하는지 이번 과제를 통해 확실히 알았습니다🤩
최상단 컴포넌트에 정의해서 props로 내려줄 필요 없이 저장해놓은 상태를 꺼내쓰기만 하면 되니까 코드도 깔끔해지고 사용하기도 매우 편리했습니다 쵝오..👍👍
📎 알게 된 부분
RecoilRoot로 최상위 컴포넌트를 감싸야 함useRecoilState또는useRecoilValue훅 사용useRecoilValue는 읽기 전용으로 상태를 가져옴useRecoilState는 상태를 읽고 쓸 수 있도록 해줌useParams는 항상 문자열로 URL 파라미터를 가져온다!→ URL에서 가져온 파라미터는
string타입으로 사용됨📎 생각해 볼 질문들
📎 남은 구현
📎 결과화면
https://jejukyj-react-sns.vercel.app/
