Skip to content
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주차] 김채림 미션 제출합니다. #16

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

chaaerim
Copy link

안녕하세요! 김채림입니다. 지난주차에 확장성을 고려하지 않은 덕에 거의 모든 코드를 뜯어고친 한 주였습니다 ..
이번주 과제를 하는 내내 고민하고 코드를 짜는 습관을 들여야겠다는 다짐을 했어요 . . 🥲

💻 배포링크
어벤져스와 채팅해보세요 . .


📗 수정한 내용
일단 message.json 구조부터 뜯어고쳤습니다. 폴더 구조도 올려주신 자료를 참고해서 수정해보았어요. 컴포넌트가 많아진만큼 공통적으로 사용하는 style-components도 많아져 CommonStyle.tsx에 공통된 스타일을 모아봤습니다. 그리고 지난주차에 react-custom-scrollbars 라이브러리를 사용해서 구현했던 스크롤바를 useRef를 이용해서 구현해봤습니다.


✏️어려웠던 부분
메세지를 어떻게 넘겨야 할지에 대한 고민이 가장 컸습니다. 아마 상태관리 라이브러리를 공부하고 사용했으면 훨씬 쉽고 효율적이었을 것 같아요 ㅎㅎ.....React Router로 렌더링하는 컴포넌트에 prop 전달을 할까 하다가 끊임없는 오류로 인해 결국 useParams()를 이용해서 링크에서 userName 파라미터 값을 가져와서 사용했습니다.

그런데 useParams()으로 가져온 값을 컴포넌트 간 props로 전달하려 하니 아래와 같은 에러가 발생했습니다.

image

interface IUserNavProps {
  currentUser: ICurrentUser;
  setCurrentUser: (element: ICurrentUser) => void;
  otherUser: Readonly<Params<string>>;
}

구글링을 해봐도 관련된 내용이 많이 없어 당황했는데 다행히 에러가 구체적으로 나와 위와 같이 인터페이스에 추가해주었더니 해결되었습니다.


😅 아쉬운 부분
Recoil을 사용해보고 싶었는데 시간이 부족하여 이 부분을 적용하지 못한 것이 아쉽습니다. 주말 동안 최대한 상태관리 라이브러리에 대해 공부를 해보려고 해요.. 그리고 기능을 구현하는데만 집중하여 렌더링 성능을 고려하지 못했습니다. 코드리뷰 반영하면서 React.memo와 useCallback 을 사용해서 렌더링도 최적화할 수 있도록 해보겠습니다 😭


⁉️ Key Questions

1. SPA
SPASingle Page Application을 의미합니다. 한 개의 페이지로 이루어진 애플리케이션이라는 의미를 가집니다. 그렇지만 싱글 페이지라고 해서 화면이 한 종류인 것은 아니고 해당 페이지에서 로딩된 js와 현재 사용자 브라우저의 주소 상태에 따라 다양한 화면을 보여줄 수 있습니다.

2. Routing
다른 주소에 다른 화면을 보여주는 것을 Routing이라고 합니다. 리액트 라이브러리 자체에 이 기능이 포함되어있지는 않아 라이브러리를 이용하여 작업을 구현합니다. 라이브러리로는 react-router를 많이 사용합니다.

3. SSR (어렵네요 . . .)
SPA 사용시 자바스크립트가 실행될 때까지 흰 페이지가 나타나는 경우가 있는데 이는 server-side-rendering을 통해 해결할 수 있습니다. SSR은 UI를 서버에서 렌더링 하는 것을 의미합니다. 리액트는 기본적으로 클라이언트 사이드 렌더링, 즉 UI 렌더링을 브라우저에서 모두 처리합니다. SSR을 구현하면 서버 쪽에서 초기 렌더링을 대신 해주게 되고, 사용자가 html을 전달받을 때 내부에 렌더링된 결과물이 보이게 됩니다. SSR을 사용할 경우 초기 렌더링 성능을 개선할 수 있지만, 결국 브라우저가 해야 할 일을 서버가 대신 해주는 것이므로 서버 리소스가 사용된다는 단점이 있기도 합니다.

@chaaerim chaaerim changed the title Chaaerim [5주차] 김채림 미션 제출합니다. May 13, 2022
Copy link
Member

@9yujin 9yujin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요, 코드리뷰파트너 한규진입니다!! 점점 어려운 내용들이 들어가다보니 코드리뷰하기가 어렵네요. 그래도 열심히 코드 읽으면서 몇가지 코멘트 남깁니다.
저도 마블영화 좋아합니다. 닥스 보셨나요? 영화 마지막에 닥스가... 더보기

import styled from 'styled-components';
import { HeaderContains, HeaderText } from '../components/layout/CommonStyle';
import UnderNavBar from '../components/layout/UnderNavBar';
import Messages from '../components/ChatList/Chattings';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컴포넌트 함수명은 Chattings인데 Messages로 가져와서 조금 헷갈리네요ㅠㅠ

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

죄송합니다.. 컴포넌트가 많아지면서 네이밍을 신경쓰지 못한것같아요 .. 반영해서 수정해볼게요!!!

<ListContainer ref={scrollbarRef}>
<ShowList>
{messageData.map((chat: IMessageData) => (
<SingleMessage chat={chat} key={chat.userId} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

userId를 key값으로 주면 동일한 값으로 들어가게 되네요. 데이터에 chatId와 같은 값을 추가하는게 좋을것 같습니다.

);
};

export default SingleMessage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

새 채팅을 입력할때마다 기존 모든 singleMessage가 다시 렌더링됩니다. 물론 아시겠지만 React.memo를 이용해서 방지할 수 있을것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리액트 devtool로 렌더링 상태 확인하고 한숨밖에 안나와서 ...ㅋ.......ㅋ...바로 수정해봤어요 ㅎㅎ....

Comment on lines +17 to +19
return element.userName
.toUpperCase()
.includes(searchText.toUpperCase());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

대소문자 상관없이 검색하는것 맞나요?? 전 생각도 못했던 디테일이네요

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻👍🏻

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이름에 대소문자를 섞어놔서 이렇게 만들어봤습니다 🥳

Copy link
Member

@corinthionia corinthionia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 채림 님 🙌🏻

오랜만에 코드 리뷰를 해 드렸습니다! 이번주에 코드를 뜯어 고치셨다 했는데 짧은 기간 안에 완성도 있게 구현해 주셨네요 👍🏻 리뷰를 달다 보니 생각보다 많이 달게 되었는데 확인해 보시면 좋을 것 같습니다!

과제하시느라 고생 많으셨고 스터디 시간에 뵙겠습니다! 😀

Comment on lines +4 to +6
import FriendList from './routes/FriendList';
import ChatList from './routes/ChatList';
import Setting from './routes/Setting';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

페이지와 관련된 컴포넌트들은 보통 pages 폴더에 작성합니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 !! 수정해보겠습니다!!!

Comment on lines +32 to +34
const ChatListContainer = styled.div``;

const Content = styled.div``;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위에서 말씀드린 것처럼 의미없는 div 태그를 사용하는 것보다는 fragment를 사용하시는 게 더 좋을 것 같아요

Comment on lines +10 to +26
<ChatListContainer>
<HeaderContains>
<BsChatDots size={20} />
<HeaderText>Message</HeaderText>
</HeaderContains>
<Content>
{message.map((userMessage) => (
<Messages
key={userMessage.userId}
userId={userMessage.userId}
userName={userMessage.userName}
message={userMessage.messages[userMessage.messages.length - 1].text}
/>
))}
</Content>
<UnderNavBar />
</ChatListContainer>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래와같이 작성하셔도 똑같이 보인답니다!

Suggested change
<ChatListContainer>
<HeaderContains>
<BsChatDots size={20} />
<HeaderText>Message</HeaderText>
</HeaderContains>
<Content>
{message.map((userMessage) => (
<Messages
key={userMessage.userId}
userId={userMessage.userId}
userName={userMessage.userName}
message={userMessage.messages[userMessage.messages.length - 1].text}
/>
))}
</Content>
<UnderNavBar />
</ChatListContainer>
<>
<HeaderContains>
<BsChatDots size={20} />
<HeaderText>Message</HeaderText>
</HeaderContains>
{message.map((userMessage) => (
<Messages
key={userMessage.userId}
userId={userMessage.userId}
userName={userMessage.userName}
message={userMessage.messages[userMessage.messages.length - 1].text}
/>
))}
<UnderNavBar />
</>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

쓸데없는 스타일 컴포넌트만 잔뜩 만들었었군요... 짚어주셔서 감사합니당...

Comment on lines +16 to +21
{message.map((userMessage) => (
<Messages
key={userMessage.userId}
userId={userMessage.userId}
userName={userMessage.userName}
message={userMessage.messages[userMessage.messages.length - 1].text}
Copy link
Member

@corinthionia corinthionia May 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 개인적으로 배열의 이름은 복수형으로 짓고, 콜백함수의 props는 단수형으로 작성하는 편이 조금 더 직관적인 것 같아서 이런 식으로 사용합니다...!

Suggested change
{message.map((userMessage) => (
<Messages
key={userMessage.userId}
userId={userMessage.userId}
userName={userMessage.userName}
message={userMessage.messages[userMessage.messages.length - 1].text}
{messages.map((message) => (
<Messages
key={message.userId}
userId={message.userId}
userName={message.userName}
message={message.messages[userMessage.messages.length - 1].text}

아니면 아래처럼 destructuring을 사용하기도 합니다

Suggested change
{message.map((userMessage) => (
<Messages
key={userMessage.userId}
userId={userMessage.userId}
userName={userMessage.userName}
message={userMessage.messages[userMessage.messages.length - 1].text}
{message.map(({ userId, userName, messages }) => (
<Messages
key={userId}
userId={userId}
userName={userName}
message={messages[userMessage.messages.length - 1].text}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네.. 코드가 길어지면서 저도 이번 과제하면서 네이밍이 참 어렵고 맘에 안들었는데 ..... 팁 감사합니다 💗 반영해서 수정해볼게요!!!!

Comment on lines +17 to +19
return element.userName
.toUpperCase()
.includes(searchText.toUpperCase());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻👍🏻

Comment on lines +69 to +71
border-color: #c2bbbb;
border-width: 0.08rem;
border-style: solid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 세 줄을 border: 0.08rem solid #c2bbbb; 로 한 번에 쓸 수 있습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저번에 효정님이 짚어주셨는데 또 이런짓을 했군요 수정하겠습니다 ㅠ ㅠ

Comment on lines +22 to +29
${({ userName }) =>
userName === 'chaaerim'
? css`
flex-direction: row-reverse;
`
: css`
justify-content: flex-start;
`}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 하면 더 간결하게 쓸 수 있습니다

Suggested change
${({ userName }) =>
userName === 'chaaerim'
? css`
flex-direction: row-reverse;
`
: css`
justify-content: flex-start;
`}
flex-direction: ${({ userName }) =>
userName === 'chaaerim' ? 'row-reverse' : 'row'};

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ㄷ ㅐ박..8줄이 2줄이 되는 매직이네요.. 더 공부해봐야겠어요 !!!

import styled from 'styled-components';

export const HeaderContains = styled.div`
height: rem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수치가 빠져 있네요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프짱님에게 딱걸렸군뇨....

Comment on lines +8 to +10
const userindex = user.findIndex(
(user) => user.userName === otherUser.userName
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter 함수를 사용하는 것도 좋을 것 같아요

{
"userId": "user0",
"userName": "chaaerim",
"text": "과제는 미리미리",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ㅜㅅㅜ

@chaaerim chaaerim closed this May 15, 2022
@chaaerim chaaerim reopened this May 15, 2022
Copy link

@siwonblue siwonblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 채림님
코드리뷰 파트너 전시원입니다.
리코일 사용하지 않고도 구현하신 점이 신기했는데 이렇게 하셨군요
저번에 처음 배포하셨을 때 문제 없이 돌아가는 걸 확인했었고 코드리뷰는 좀 뒤늦게 하는데
다른 분들 리뷰를 반영하면서 버그가 생긴 것 같아요 ! 추가로 반영하시면 문제없이 돌아갈 것 같네요

스터디때 뵐게요


const handleInputSubmit = (e: React.FormEvent<HTMLFormElement>) => {
//공백이 아닐 때에만 send 가능
if (textinput.replace(/\s+/g, '')) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required 속성도 괜찮아요 !

Comment on lines +6 to +9
button {
&:hover{
cursor: pointer;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

버튼까지 전역으로 스타일링 좋네요

"react-scripts": "5.0.1",
"recoil": "^0.7.3-alpha.2",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

시도의 흔적

Comment on lines +12 to +19
<LinkToChat to={`/messengerbox/${userName}`}>
<ProfileImg src={`/assets/${userId}.jpg`} />
<ListItem>
<UserName>{userName}</UserName>
<LastMessage>{message}</LastMessage>
</ListItem>
</LinkToChat>
</ChattingRooms>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 방식이라면 확실히 리코일 없이 데이터가 관리가 편했겠네요.
저도 지난 주에는 이런 식으로 했었는데 편했는데 이번주에 리코일로 관리를 해봤거든요
전역으로 다룰 수 있어서 좋았습니다. 리덕스보다 훨씬 편리해서 추천드려용

@@ -0,0 +1,43 @@
import styled from 'styled-components';
import { BsSearch } from 'react-icons/bs';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꿀팁 배워갑니다

//새로고침 방지
e.preventDefault();
},
[messageData]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[messageData,textinput]
textinput 을 추가안해주면 입력값이 공백으로 되네요
setState 가 비동기적으로 동작해서 생기는 문제 같아요

value={textinput}
onChange={handleInputChange}
placeholder="Send Message . . ."
type="text"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required 속성을 추가해줘도 편하더라구요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants