-
Notifications
You must be signed in to change notification settings - Fork 10
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주차] 정대헌 미션 제출합니다. #20
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.
안녕하세요. 코드리뷰 파트너 주효정입니다! 우선 말풍선의 시작과 끝 다르게 표현하는 부분 저도 구현해보고 싶었는데 이렇게 구현해주셔서 너무 잘 봤어요! 그리고 저는 예전에 redux만 사용해봤었는데 redux toolkit 사용하신 코드 보니까 저도 redux toolkit 사용해서 한번 구현해보고 싶은 마음이 생겼어요👍 코드 흥미롭게 잘 봤습니다!!
코드 보면서 생각난 점들 코멘트로 조금 남겼습니다! 수고 많으셨습니다.
|
||
const AppContainer = styled.div` | ||
display: flex; | ||
flex-direction: row; |
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.
flex-direction
의 default가 row
라 이 부분은 생략해도 되지 않을까요?!
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.
맞아요~ 본능적으로 쓰게되는.. 실수입니다.
<Component onClick={()=>{ | ||
dispatch(setActiveChat(chat.chatId)); | ||
navigate(`/chats/${chat.chatId}`); | ||
}}> |
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.
맞습니다, 참조하도록 하겠습니다!
{chat.clients.map((client)=>{ | ||
if(client.id !== clients[0].id) | ||
return( | ||
<ProfileDiv key={chat.chatId}> | ||
<ProfileImage src={client.imageUrl}/> | ||
<NameDiv>{client.name}</NameDiv> | ||
</ProfileDiv> | ||
) | ||
})} |
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.
{chat.clients.map((client)=>{ | |
if(client.id !== clients[0].id) | |
return( | |
<ProfileDiv key={chat.chatId}> | |
<ProfileImage src={client.imageUrl}/> | |
<NameDiv>{client.name}</NameDiv> | |
</ProfileDiv> | |
) | |
})} | |
{chat.clients.map( | |
(client) => | |
client.id !== clients[0].id && ( | |
<ProfileDiv key={chat.chatId}> | |
<ProfileImage src={client.imageUrl} /> | |
<NameDiv>{client.name}</NameDiv> | |
</ProfileDiv> | |
) | |
)} |
&&연산자 사용하고 return
문도 생략해주면 조금 더 간단하게 작성할 수 있지 않을까요??
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.
&& 연산자가 익숙하지 않은데, 보기 더 간편해지는 것 같아요. 좋습니다~
max-width: calc(100% - 4rem); | ||
height: 100%; | ||
background-color: ${({selected})=>(selected? "#ff9a9e":"#ffecd2")}; | ||
border-radius: ${({selected, manner})=>(selected? bubbleRadiusSelected[manner]:bubbleRadiusDeselected[manner])}; |
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.
감사합니다~
return( | ||
<Component selected={selected} manner={manner}> | ||
<MessageProfileDiv> | ||
{manner === MessageType.BASE || manner === MessageType.HEAD? <MessageProfileImg src={msg.client.imageUrl}/> : "" } |
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.
{manner === MessageType.BASE || manner === MessageType.HEAD? <MessageProfileImg src={msg.client.imageUrl}/> : "" } | |
{(manner === MessageType.BASE || manner === MessageType.HEAD) && ( | |
<MessageProfileImg src={msg.client.imageUrl} /> | |
)} |
이부분도 &&연산자 사용하면 더 간략하게 적을 수 있을 것 같아요~!
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.
넵, 감사합니다!
<ProfileButton onClick={()=>{ | ||
const id = uuidv4(); | ||
dispatch(addChat({ | ||
chatId: id, | ||
activeClient: clients[0], | ||
clients: [clients[0], client], | ||
messages: [], | ||
})); | ||
dispatch(setActiveChat(id)); | ||
dispatch(setActiveClient(clients[0])); | ||
navigate(`/chats/${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.
이 부분도 onClick
메서드가 너무 길어지는 것 같아서 따로 빼는게 좋을 것 같아요!
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 handleTimeFormat = (time: Date):string[] => { | ||
const hourString: string = time.getHours().toString(); | ||
const minString: string = time.getMinutes().toString(); | ||
const timeString: string = (hourString.length === 2? hourString : "0" + hourString) + ": " + (minString.length === 2? minString : "0" + minString) |
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 timeString: string = (hourString.length === 2? hourString : "0" + hourString) + ": " + (minString.length === 2? minString : "0" + minString) | |
const timeString: string = hourString.padStart(2, "0") + ":" + minString.padStart(2, "0"); |
padStart
를 사용해서 더 간단하게 표현할 수 있을 것 같아요~!
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.
padstart 신기하더라구요, 훨씬 보기 좋은 것 같습니다.
if(message !== "" && activeClient){ | ||
|
||
const [timeString] = handleTimeFormat(new Date()); | ||
|
||
dispatch(addMessage({ | ||
msgId: uuidv4(), | ||
client: activeClient, | ||
content: message, | ||
timeString: timeString, | ||
messageType: MessageType.BASE, | ||
})); | ||
|
||
setMessage(""); | ||
}else if(message === ""){ | ||
alert("enter the 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.
if(message !== "" && activeClient){ | |
const [timeString] = handleTimeFormat(new Date()); | |
dispatch(addMessage({ | |
msgId: uuidv4(), | |
client: activeClient, | |
content: message, | |
timeString: timeString, | |
messageType: MessageType.BASE, | |
})); | |
setMessage(""); | |
}else if(message === ""){ | |
alert("enter the message") | |
if(message.trim() && activeClient){ | |
const [timeString] = handleTimeFormat(new Date()); | |
dispatch(addMessage({ | |
msgId: uuidv4(), | |
client: activeClient, | |
content: message, | |
timeString: timeString, | |
messageType: MessageType.BASE, | |
})); | |
setMessage(""); | |
}else if(!message.trim()){ | |
alert("enter the 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.
맞습니다. trim함수 참조하도록 할게요!
element={ | ||
<main style={{ padding: "1rem" }}> | ||
<p>NOT FOUND</p> | ||
</main> | ||
} |
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.
Not Found 화면도 설정해두셨네요!👍 코드가 길지는 않지만 이 부분도 한 페이지로 따로 빼도 괜찮을 것 같아요!
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.
네, 참조하겠습니다~
{chats.map(chat=>{ | ||
return( | ||
<ChatButtonComponent key={chat.chatId} chat={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.
{chats.map(chat=>{ | |
return( | |
<ChatButtonComponent key={chat.chatId} chat={chat}/> | |
) | |
})} | |
{chats.map(chat => ( | |
<ChatButtonComponent key={chat.chatId} chat={chat}/> | |
))} |
return
문 생략해도 좋을 것 같아요!
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.
안녕하세요 대헌님
코드리뷰 파트너 전시원입니다.
디테일한 부분 많이 배워갑니다.
읽으면서 좋았던 부분 다 코멘트하면 30개 넘어갈 것 같네요 ^^;
채팅방에서 새로고침을 하면 데이터를 못 불러와서 오류가 발생하는 것 같아요
optional chaining 을 이용해보려 했는데 정확히 어떤 부분에서 문제가 생겼는지 파악이 힘들어
수정을 못했습니다. uuid 를 이용해서 url param 으로 랜덤 값을 넘기다 보니 못 불러오는 것 같기도 하구요.
항상 대헌님 코드보면서 감탄하고 많이 배웁니다.
스터디때 뵐게요
|
||
const handleFormSubmit = (message: string, e?:React.FormEvent<HTMLFormElement>) => { | ||
|
||
if(e) e.preventDefault(); |
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.
하하 감사합니다
<ProfileDiv key={client.id}> | ||
<ProfileButton onClick={()=>{ | ||
const id = uuidv4(); | ||
dispatch(addChat({ | ||
chatId: id, | ||
activeClient: clients[0], | ||
clients: [clients[0], client], | ||
messages: [], | ||
})); | ||
dispatch(setActiveChat(id)); | ||
dispatch(setActiveClient(clients[0])); | ||
navigate(`/chats/${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.
이 부분은 혹시 map 에서 사용하는 client 때문에 밖으로 빼지 않으셨나요?
저도 비슷한 경우로 함수를 인라인으로 사용한 경우가 있어서요 ! (사실 어떻게 빼야할지 모르겠더라구요 ㅠ)
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.
맞아요, 외부 함수로 정의하면 되지만 마감 시간의 압박으로...
function InputContainer() { | ||
const [message, setMessage] = useState(""); | ||
|
||
const activeClient:Client = useAppSelector((state)=>state.chat.chats[state.chat.acidx!].activeClient); |
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.
맞습니다. uid를 링크로 직접 삽입하는 방식이다 보니
채팅방 정보를 localStorage를 저장하는 방식으로 이용해야 할 것 같아요!
문제 해결을 위해 함께 고민해주셔서 감사합니다 :)
` | ||
|
||
function ProfileContainer() { | ||
const {clients, activeClient} = useAppSelector((state)=> state.chat.chats[state.chat.acidx!]) |
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.
새로고침하면 이 부분에서 문제가 발생하네요 (2)
|
||
function MessageContainer() { | ||
|
||
const messages = useAppSelector((state)=> state.chat.chats[state.chat.acidx!].messages); |
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)
<Routes> | ||
<Route path="/" element={<App/>}> | ||
<Route path="chats" element={<ChatList/>}/> | ||
<Route path="chats/:chatId" element={<Chatroom/>}/> |
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.
이 부분에서 새로고침하면 현재 오류가 뜨는데
NOT FOUND 구현해두신 걸 써먹지 못하니 아쉽네요 😭
비동기 처리에 대한 오류 캐치가 아니라 지금상황에선 더 좋은 방안이 생각이 안나네요 ..
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.
링크가 깨지는데도 NOT FOUND로 연결되지 않는 점이 이상하긴 하더라구요.
아마 /:value 식으로 동적 할당하면 react router가 값이 없다고 반환하지 않고 값을 찾으려고 하는 것 같습니다.
border-radius: 3rem; | ||
background-color: rgba(255, 255, 255, 0.5); | ||
display: flex; | ||
margin: 0.8rem 0 0.8rem 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.
효정님께서도 언급하셨지만
두 개로 줄인다면 상하를 조정할 수 있으니 줄이셔도 좋겠네요
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.
네 맞습니다~!!
배포링크
https://react-messenger-15th-asiloveyu.vercel.app/friends
설명
필수 요건
결과 화면과같이 구현합니다.
친구 목록 페이지, 채팅 목록 페이지, 설정 페이지 세 부분으로 구성합니다.
예제 페이지와 동일한 플로우를 가지고 있습니다.
채팅 목록을 누르면 4주차 미션에서 구현했던 채팅방으로 이동합니다.
시간 관계상 아직도 JSON 로딩을 추가하지 못해...아쉬움이 남습니다.
로컬에서 API 연결을 가상화 하는 패키지도 있어서, 시도해보고 싶었는데 연습으로 괜찮은 것 같습니다.
링크를 잃어버려서 다시 찾게되면 업데이트 하겠습니다.
검색 기능을 추가하여 검색한 내용과 일치하는 이름을 가진 사용자만 화면에 표시합니다.
useState를 이용해서 가장 기본적인 형태로 간단하게 처리할 수 있었습니다.
다만, 단어 입력 시 플리커링이 발생하는 것을 보아, 계속 리프레시 되는 것을 확인 할 수 있었습니다.
조금 더 효율적인 방법이 있을 것 같아 다른 분들의 코드를 참조하면 좋을 것 같습니다.
(선택) 각자 메신저에 추가하고 싶거나, 구현하고 싶은 기능 마음껏 구현합니다. ✨
메시지의 UI 구성에 시간을 쓰게 되었습니다. 아이폰에서 보이는 시작-중간-끝이 서로 다른 형태로 나타나는 점이요!
왜 이런데 시간을 사용했는지 후회스럽지만 다들 관심있으신 것 같아, 함께 코드 공유하면 좋을 것 같아요.
추가로, 메시지 방과 프로필 간의 동적인 관계를 구현해보려고 했습니다.
각 메시지 방은 고유한 주소를 가집니다. (같은 사용자에게도 여러 개의 방을 개설할 수 있습니다)
사용자 명단을 통해 들어가면 새로운 방이 생성되고, 방 명단의 방은 생성된 방들로 연결됩니다.
많은 분들이 고민하셨던 단체방 생성도 로직 상 가능하지만 아직 UI로 구현은 하지 않았습니다.
선택 사항
Recoil, Redux 등의 상태 관리 라이브러리를 적용해 봅니다.
Redux Toolkit 을 사용중입니다.
Redux의 장점을 활용해 메시지 전체를 상태관리로 저장하고 있습니다.
메신저가 생각해야 될 점이 정말 많더라구요. 또한, 상태관리 로직을 정확히 이해하는 것에 대해서 갈수록 더 절실히
느껴지는 것 같습니다. 각자 정말 다른 방식으로 구현 하셨을 것 같아, 함께 나누면 좋을 것 같아요.
Key Questions
Routing
Routing에 대해서도 긴 documentation이 있더라구요. 웹페이지 사용할때는 몰랐는데 생각보다 많은 고민과 이해가 필요한 것 같습니다.
전통적인 HTML 문서에서는 사용자가 링크를 클릭하거나 앞뒤로 이동, 또는 양식을 제출할 때 서버에 페이지를 새로 요청합니다. 앞서 접속한 링크는 브라우저의 히스토리 스택에 저장됩니다. 스택에서는 사용자가 앞뒤 이동에 따라 push, pop, replace 이벤트가 발생합니다.
반면 React router와 같은 CSR에서는 개발자가 브라우저의 히스토리 스택을 직접 제어할 수 있습니다. 예를들어 웹 API에서는 window.history.pushstate()함수를 제공하고 있습니다. 다만, 이러한 함수가 새로운 페이지를 로드하지는 않습니다. 그래서 React Router의 history 오브젝트는 URL 변화에 반응하여 re-render를 지시합니다.
(URL이 Uniform Resource Locator의 약자라고 하네요, 전혀 예상하지 못한 뜻 입니다.)
React router의 URI은 3가지 요소(pathname, search, hash)로 구성됩니다. 우선 pathname은 route 컴포넌트를 통해 하나씩 등록(match)하는 그 영역입니다. Search는 query string을 저장하는 영역으로 URL에서 '?'를 통해 구분됩니다. Query string이란 데이터베이스에 전달하는 데이터입니다. 대표적으로 input 요소를 통해 받는 검색 쿼리, 혹은 form 요소를 통해 받는 이름, 나이 등이 있을 수 있겠네요. 마지막으로 Hash란 페이지의 위치에 대해 전달하는 값으로 React router에서는 스크롤 포지션을 저장하는데 사용합니다.
(히스토리 스택 접근이 불가능하던 과거 웹 개발에서는 hash를 CSR를 구현하기 위한 꼼수로 사용했다고 하네요.)
브라우저는 URL 이외에도 숨겨진 state를 통해 사용자의 위치를 저장합니다. Web API의 pushState() 함수의 첫번째 인자를 통해 state를 제어합니다. State를 이용하면 접근 루트를 추적하거나 인자(상태정보)를 전달할 수 있습니다. 대표적인 예로 Pinterest에서 grid view에서 접속하면 modal view로 들어가지만, url을 복붙하면 독자 ui view로 들어가는 경우가 있습니다. React router의 함수들은 이 state를 접근하는 인자도 제공합니다.
추가로 이번 과제와 같이 ':id'와 같은 문법으로 route를 정의하는 것을 dynamic segmentation이라고 합니다. 말그대로 동적으로 URL을 등록하는 것을 말합니다. 이때 공식 문서는 재밌는 문제를 제시하는데요, "/teams/new"가 "/teams/new, /teams/:teamId" 두가 패턴에 부합한다는 것입니다. ":teamId"는 어떤 string도 받아들일 수 있습니다. React router v5까지는 이 문제를 route confing상 순서가 빠른 순으로 처리했지만, 현재는 가장 exact match를 찾아서 등록한다고 하네요.
이외에도 즐거운 내용들이 많이 있으니 https://reactrouter.com/docs/en/v6/getting-started/concepts#defining-routes 참조하시면 좋을 것 같아요.
SSR
HTML 규격의 등장 이후 SSR은 유일한 규격이었습니다. 개발자가 HTML 파일을 서버에 업로드 하면 서버는 파일을 처리해 사용자에게 렌더링이 끝난 문서를 브라우저를 통해 보여주었습니다. 웹이 단순하던 시절에는 문제가 없었지만, 모던 웹의 동작은 앱에 가깝습니다. 페이지의 렌더링 횟수가 증가할 수록 SSR의 느린 속도는 문제시 되었습니다. SSR은 페이지에 변화 정도와 무관하게 완전히 새로운 페이지를 요구하기 때문입니다.
단, SSR은 SEO 노출에 유리하다는 장점을 가지고 있습니다. 서버에서 렌더링이 끝난 문서를 전달하므로 크롤러가 해당 정보를 토대로 인덱싱할 수 있습니다.
반면 CSR은 자바스크립트를 이용해 브라우저 상에서 렌더링을 실행하는 것을 말합니다. 완성된 html 문서와 빈 html 문서이지만 js로 구동되는 구조를 비교 예시로 들 수 있습니다. 이제 서버는 이 빈 문서의 boiler plate만 렌더링하면 되는군요. 또한 새로운 페이지를 로드할 때도 일부 영역만 새로 렌더링 할 수 있습니다. 다만 SEO 노출 상 한계와 최초 로드 시간이 길어진다는 단점이 존재합니다.
https://www.freecodecamp.org/news/what-exactly-is-client-side-rendering-and-hows-it-different-from-server-side-rendering-bd5c786b340d/
SPA
SPA는 새 페이지를 불러오지 않고 현제 페이지를 동적으로 재 작성하는 웹앱 패턴을 말합니다. SPA의 대표적인 방식은 AJAX 이용은 서버로부터 html 파일을 새로 불러오는 것이 아니라 JSON 데이터를 받아와 동적으로 렌더링 합니다.