-
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
[5주차] 고주희 미션 제출합니다. #15
base: main
Are you sure you want to change the base?
Conversation
JavaScript를 TypeScript로 변경
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주차 미션 코드 리뷰를 맡게 된 심여은입니다!
우선 다소 늦게 리뷰해드려 죄송합니다😭 한번에 이해하지 못한 코드들이 있어 꼼꼼하게 살펴보았는데요, js의 다양한 메서드들을 잘 사용하셔서 리뷰를 하며 오히려 제가 많이 얻어갈 수 있었습니다. 또한 복잡한 json 객체에 대해서도 recoil의 useRecoilState를 잘 사용하셨어요!
이번 프로젝트 하느라 정말 고생하셨고 다음 프로젝트도 많이 기대하고 있겠습니다!
export const router = createBrowserRouter([ | ||
{ | ||
path: '/', | ||
element: <App />, | ||
children: [ | ||
{ | ||
path: 'chats', | ||
element: <Chats />, | ||
}, | ||
{ | ||
path: 'chat/:roomId', | ||
element: <Chat />, | ||
}, | ||
], | ||
}, | ||
]); |
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 Router v6에서 새롭게 도입된 API네요. 각 route에 대해 상세한 정의를 작성할 수 있어 좋은 코드인 것 같아요!
<Img.RoundIcon | ||
width="36px" | ||
height="36px" | ||
src={receiver?.profilePicture} | ||
alt="profile" | ||
/> |
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.
이런 식으로 Img, Text 등 자주 사용되는 공통 style들을 따로 정리해서 놓은 점 매우 좋아요! 저도 img 컴포넌트에서 공통되는 css 코드들이 많았는데, 이렇게 정리해보도록 하겠습니다!
import { UserState } from '../../atoms'; | ||
|
||
export default function Message({ message, isOwner }) { | ||
const [users, setUser] = useRecoilState(UserState); |
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가 새롭게 추가되거나 삭제되는 기능이 없어서, user를 상태로 관리해야 할 필요성을 잘 모르겠습니다.
users를 useRecoilValue가 아닌 useRecoilState로 초기화한 이유가 있을까요?
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.
저도 궁금했습니다 !
room.messages.some( | ||
(message) => | ||
message.userId === participant.userId && | ||
message.userId !== OWNER_USER_ID | ||
) |
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.
오 some() 함수를 사용하면 주어진 조건을 만족하는 user를 쉽게 구할 수 있군요! some()과 every() 함수에 대해 잘 몰랐는데 새롭게 알아갑니다!
export default function useInput<T>( | ||
initialValue: T | ||
): [ | ||
T, | ||
( | ||
event: React.ChangeEvent< | ||
HTMLInputElement | HTMLTextAreaElement | HTMLSelectElement | ||
> | ||
) => void, | ||
Dispatch<SetStateAction<T>> | ||
] { |
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 hook의 dispatch를 사용했는데, 이걸 recoil로 바꿔 적용할 수 있나요?
"isOwner": true, | ||
"userId": "hijh_0522", | ||
"content": "그러게", | ||
"timestamp": "2024-10-03 18:09:00" |
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를 실제 아이디처럼 작성한 부분이 좋아요🥰
const participantIds = new Set( | ||
filteredMessages.flatMap((msg) => msg.messages.map((m) => m.userId)) | ||
); |
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.
이렇게 작성하면 messages의 중첩 배열을 펼치고, userId만 추출한 후, set으로 중복 요소를 제거할 수 있네요!
이 방법도 좋고, 아니면 채팅에 참여한 userId만 정의한 배열을 Message 객체의 속성으로 추가하는 방법도 있습니다😃
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.
안녕하세요. 주희 님 ~! 이번 주차 리뷰어 김윤일입니다 😊 이번주차 과제 빨리해 주셨는데 늦게 리뷰해 드려서 죄송합니다 ! 한창 바쁠 시기에 열심히 하시는 모습 정말 보기 좋아요 👍🏻
주희 님의 재사용성을 고려한 코드 잘 봤습니다. 개발하다 보면 시야가 좁아질 수도 있는데 항상 숲을 보면서 코딩하시는 거 같아요. 배울 점이 많습니다! 홧팅홧팅 👍🏻
import { UserState } from '../../atoms'; | ||
|
||
export default function Message({ message, isOwner }) { | ||
const [users, setUser] = useRecoilState(UserState); |
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.
저도 궁금했습니다 !
createRoot(document.getElementById('root') as HTMLElement).render( | ||
<StrictMode> | ||
<RecoilRoot> | ||
<ThemeProvider theme={theme}> |
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.
테마를 전역관리로 해서 유지보수가 쉽게 해주셨군요 !
저도 활용해봐야겠어요 ~👍🏻
build: { | ||
rollupOptions: { | ||
external: ['/@emotion/react/jsx-runtime'], | ||
}, |
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.
이 옵션은 주로 번들 크기 최적화나 특정 라이브러리를 애플리케이션 외부에서 따로 로딩하려는 상황에서 사용할 때 사용한다고 알고 있는데 주희님은 어떤 목적으로 사용하신 건가요 ?
HTMLInputElement | HTMLTextAreaElement | HTMLSelectElement | ||
> | ||
) => void, | ||
Dispatch<SetStateAction<T>> |
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.
직접 변경 하기 위해 dispatch를 사용하신 건가요 ?
pointer?: boolean; | ||
}; | ||
|
||
export const 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.
프로젝트 규모가 크면 클수록 재사용성이 높은 게 중요하다고 생각합니다. 주희 님은 항상 재사용을 고려하면서 개발하시는 거 같아 멋있습니다!!!
"roomId": 1, | ||
"messages": [ | ||
{ | ||
"isOwner": 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.
isOwner로 어떤 메세지가 본인인지 구분할 수 있게 해두셨군요 !
가독성 굿굿 !
@@ -0,0 +1,168 @@ | |||
import { atom } from 'recoil'; | |||
|
|||
interface IUser { |
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.
이렇게 인터페이스 이름 앞에 I
를 붙이는 형식을 헝가리안 기법이라고 하는데, 최근에는 조금 지양하는 방법이라고 해요!
참고자료 - TypeScript의 interface와 type에 대한 컨벤션
import React, { useEffect, useRef } from 'react'; | ||
import Message from '../Message/Message'; | ||
|
||
export default function ChatBody({ filteredMessages }) { |
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.
props
에 대한 type도 명시하면 좋을 것 같아요!
{filteredMessages[0].messages.map((message, index) => { | ||
return ( | ||
<div | ||
key={index} |
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.
key
값으로는 index
말고 각 데이터의 고유한 값을 넣어 주세요!
일반적으로 데이터의 id
값을 넣어 주는데, 여기에서는 id
값이 없으니 timestamp
값을 대신 사용해도 될 것 같아요 👀
export default function ChatHeader({ receiver }) { | ||
const navigate = useNavigate(); | ||
const handleBackButtonClick = () => { | ||
navigate('/chats'); |
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.
보통 '뒤로가기'를 구현하는 경우에는 navigate(-1)
과 같은 형태로 구현합니다! 이렇게 서비스 내에 뒤로가기 버튼 외에도 브라우저의 뒤로가기 버튼이 존재하기 때문에, 브라우저 뒤로가기 버튼과 동일하게 동작하도록 하려면 navigate(-1)
를 사용해야 하기 때문입니다!
<form onSubmit={handleFormSubmit}> | ||
<Style.TextAreaWrapper | ||
type="text" | ||
value={inputValue} | ||
onChange={handleChange} | ||
placeholder="메세지 보내기" | ||
autoFocus | ||
/> | ||
</form> | ||
<Style.SendMessageLogoWrapper onClick={handleClick}> | ||
<Img.RoundIcon | ||
width="22px" | ||
pointer | ||
src="/images/plane.svg" | ||
alt="send" | ||
/> | ||
</Style.SendMessageLogoWrapper> |
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.
SendMessageLogoWrapper
를 div
가 아닌 button
으로 구현하고 type="submit"
속성을 부여하면 handleClick
을 별도로 구현하지 않아도 될 것 같아요!
<form onSubmit={handleFormSubmit}> | |
<Style.TextAreaWrapper | |
type="text" | |
value={inputValue} | |
onChange={handleChange} | |
placeholder="메세지 보내기" | |
autoFocus | |
/> | |
</form> | |
<Style.SendMessageLogoWrapper onClick={handleClick}> | |
<Img.RoundIcon | |
width="22px" | |
pointer | |
src="/images/plane.svg" | |
alt="send" | |
/> | |
</Style.SendMessageLogoWrapper> | |
<form onSubmit={handleFormSubmit}> | |
<Style.TextAreaWrapper | |
type="text" | |
value={inputValue} | |
onChange={handleChange} | |
placeholder="메세지 보내기" | |
autoFocus | |
/> | |
<Style.SendMessageLogoWrapper type="submit"> | |
<Img.RoundIcon width="22px" pointer src="/images/plane.svg" alt="send" /> | |
</Style.SendMessageLogoWrapper> | |
</form> |
SendMessageLogoWrapper: styled.button`
width: 80px;
height: 36px;
background-color: #445ffe;
border-radius: 24px;
display: flex;
justify-content: center;
align-items: center;
`,
export default function Message({ message, isOwner }) { | ||
const [users, setUser] = useRecoilState(UserState); | ||
|
||
const user = users.find((u) => u.userId === message.userId); |
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
와 같은 명시적인 이름을 써주면 좋을 것 같아요!
const user = users.find((u) => u.userId === message.userId); | |
const user = users.find((user) => user.userId === message.userId); |
|
||
const user = users.find((u) => u.userId === message.userId); | ||
|
||
const formatTimestamp = (timestamp) => { |
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.
컴포넌트의 props나 함수의 인자값은 꼭 타입을 지정해 주는 게 좋습니다!
그리고 timestamp
라는 이름은 보통 1728916619
이런 형식의 Unix timestamp를 사용할 때에만 쓰는 것 같아요 😶
const formatTimestamp = (timestamp) => { | |
const formatTimestamp = (time: string) => { |
const [messages, setMessage] = useRecoilState(MessageState); | ||
const [users, setUser] = useRecoilState(UserState); |
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.
컴포넌트 내에서 setMessage
와 setUser
는 사용되지 않고 있어서 useRecoilValue
를 사용해도 될 것 같아요!
const filteredMessages = messages.filter( | ||
(msg) => msg.roomId === Number(roomId) | ||
); |
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.
특정 roomId
를 가진 메시지 목록 하나만 찾는 것이기 때문에 find
메서드를 사용해도 좋을 것 같아요!
안녕하세요, LG U+ URECA 프론트엔드 대면반 1기 고주희입니다.
이번 주차에서는 지난 번 과제 진행에 이어 react-router-dom 을 사용하여 페이지 이동이 가능하도록 만들고 Recoil을 사용하여 Prop drilling을 피하는 것에 집중하였습니다.
전역 상태관리 필요할 것으로 예상되는 message와 user 데이터를 atoms.ts 파일에 세팅해 둔 이후 추가 기능을 구현했으면 좋았을텐데, 기존 방식(props drilling)대로 모든 기능을 구현해 두고서야 상태관리 라이브러리를 사용하는 것이 필수 과제라는 문장을 뒤늦게 보았습니다. 그리하여 다시 recoil을 사용하는 것으로 변경하려니 시간도 더 걸리고 한참 돌아온 것 같아 아쉬웠습니다. 하지만, 이 과정을 통해 오히려 더 상태 관리 라이브러리의 필요성을 극적으로 느낄 수 있었기에 의미있는 시간이었다고 생각됩니다. 🤩
또한, 지난 과제에서는 코드 확장, 재사용을 고려하여 구현하는 것이 목표였는데 그때 고려했던 흐름 그대로 확장이 진행할 수 있어서 매우 흥미로웠습니다. 매번 큰 틀에서 작게 좁혀나가는 방식으로만 개발했었는데 작은 것에서 큰 것으로 확장하는 경험을 얻을 수 있었을 뿐더러, 추후 기능 추가가 될 것을 고려하여 더 명확한 파일 이름과 구조를 잡는 것이 왜 중요한 지 몸소 느껴볼 수 있었습니다.
구현 상황
📖 생각해 볼 질문들
(1) SPA / MPA / CSR / SSR 이란 무엇인가요?
(2) React에서 상태는 어떻게 관리할 수 있나요?
(3) 어떤 상태관리 라이브러리를 사용했고, 그것의 특징은 무엇인가요?
결과 화면
배포 링크
https://zuitopia-sns.vercel.app/chats