[3주차] 이승범 미션 제출합니다. by sbl133 · Pull Request #11 · CEOS-Developers/react-messenger-13th

sbl133 · 2021-04-28T16:00:05Z

미션기간이 길었지만 학교에서 과제가 많이 나오는 시기랑 겹쳐서 좀 각박하게 미션수행을 했던거 같습니다.
나름 열심히 만들었는데 실력이 많이 부족해서 아쉬움이 많이 남아요..
react-router 존재만 알고 실제로 구현해본적이 없었는데 이번 미션을 계기로 react-router에 대해서 제대로 알게 된것 같아 많은 도움이 되었습니다. 아직 SSR과 SPA같은 네트워크(?) 개념이 부족한거 같지만 스터디전까지 열심히 공부해 보겠습니다.
감사합니다!
배포주소 : https://react-messenger-13th-eta.vercel.app/

comforest

중복 소스 코드가 조금 있네요.
데이터를 총괄적으로 관리하지 못한것도 조금 아쉬워요. (어렵긴 하죠...)
특히 아래와 같은 코드가 있던데

if(condition1) {
  state1
} else {
  if(condition2) {
    state2
  }
}

이런 코드는

if(condition1) {
  state1
} else if(condition2) {
  state2
}

처럼 하는게 더 낫겠죠? 특히 state1 이랑 state2 가 같다면

if(condition1 || condition2) {
  state1
}

로 하는게 훨씬 좋습니다

comforest · 2021-05-08T12:31:48Z

@@ -0,0 +1,58 @@
+{   
+    "black" : [
+        {"isMe": false, "text": "hey wassup You to party tonight?"},


Suggested change

{"isMe": false, "text": "hey wassup You to party tonight?"},

{"userId": 0, "text": "hey wassup You to party tonight?"},

추후 단톡방 기능과 같이 확장성을 생각하면 userId를 이용해 본인 userID 와 비교해서 isMe 여부를 파악하는게 좋을 것 같아요.

저도 이 말에 동의합니다 ㅎㅎ

comforest · 2021-05-08T12:33:54Z

+                <WhiteChat key={i}>
+                    <Text>
+                        {v.text}
+                    </Text>
+                    <ChatIcon src="/img/white.png" alt="img" />
+                </WhiteChat>
+                :
+                <BlackChat key={i}>
+                    <ChatIcon src={`/img/${id}.png`} alt="img" />
+                    <Text>
+                        {v.text}
+                    </Text>
+                </BlackChat>


같은 컴포넌트 구성이더라도
flex-direction 같은걸 이용해서 구현이 가능합니다.
말 주머니에 시간 정보를 추가하는등 여러 수정사항이 들어올 때마다
두 컴포넌트 모두 수정하기보단 css만 수정하는게 훨씬 편하겠죠?

comforest · 2021-05-08T12:35:17Z

+`;
+
+const ChattingPage = ({ match }) => {
+    const [isMe, setIsMe] = useState(true);


위에서 언급한 것 처럼 id 같은걸 사용하는게 확장성에 용이합니다

comforest · 2021-05-08T12:37:00Z

+
+
+const ChatListPage = () => {
+    const friendList = initialList;


App 에서 props로 관리해야 페이지간 데이터 관리가 가능 할 것 같아요

comforest · 2021-05-08T12:38:19Z

+            if (text === ''){
+                return <FriendState props={v} where={'FriendList'}/>;
+            } else {
+                if (v.name.includes(text)){
+                    return <FriendState props={v} where={'FriendList'}/>;
+                }
+            }


Suggested change

if (text === ''){

return <FriendState props={v} where={'FriendList'}/>;

} else {

if (v.name.includes(text)){

return <FriendState props={v} where={'FriendList'}/>;

}

}

if (text === '' || v.name.includes(text)){

return <FriendState props={v} where={'FriendList'}/>;

}

중복된 코드는 줄이는 방향으로

comforest · 2021-05-08T12:38:46Z

+        <Top>
+            <H3>친구 {friendList.length}</H3>
+            <Icon>
+            <BsPersonPlus size="24" padding-right="10"/>


Suggested change

<BsPersonPlus size="24" padding-right="10"/>

<BsPersonPlus size="24" padding-right="10"/>

들여쓰기 안되어 있네요

comforest · 2021-05-08T12:39:46Z

@@ -0,0 +1,39 @@
+import React,{} from 'react';
+import styled from 'styled-components';
+import ChatData from './ChatData.json';


역시 props로 관리 했으면 더 좋았을 것 같아요.

comforest · 2021-05-08T12:40:20Z

+            <Icon src = {img} alt="img" />
+            <div>
+                <Name>{name}</Name>
+                {where === 'FriendList' ? <Message>{message}</Message> : <Message>{lastChat}</Message>}


Suggested change

{where === 'FriendList' ? <Message>{message}</Message> : <Message>{lastChat}</Message>}

{<Message>{where === 'FriendList' ? message : lastChat}</Message>}

comforest · 2021-05-08T12:41:58Z

+            {isMe ? <TopIcon src="/img/white.png" alt="image" /> : 
+                <TopIcon src={`/img/${id}.png`} alt="image" />}


Suggested change

{isMe ? <TopIcon src="/img/white.png" alt="image" /> :

<TopIcon src={`/img/${id}.png`} alt="image" />}

<TopIcon src={isMe ? "/img/white.png" : `/img/${id}.png`} alt="image" />

중복 코드를 줄이는 방향으로

comforest · 2021-05-08T12:44:20Z

+                if (text === ''){
+                    return (
+                        <StyledLink to={`/ChattingPage/${v.id}`}>
+                            <FriendState props={v} where={'ChatList'}/>
+                        </StyledLink>
+                    );
+                } else {
+                    if (v.name.includes(text)){
+                        return (
+                            <Link to={`/ChattingPage/${v.id}`}>
+                                <FriendState props={v} where={'ChatList'}/>
+                            </Link>
+                        );
+                    }
+                }


Suggested change

if (text === ''){

return (

<StyledLink to={`/ChattingPage/${v.id}`}>

<FriendState props={v} where={'ChatList'}/>

</StyledLink>

);

} else {

if (v.name.includes(text)){

return (

<Link to={`/ChattingPage/${v.id}`}>

<FriendState props={v} where={'ChatList'}/>

</Link>

);

}

}

if (text === '' || v.name.includes(text)){

return (

<StyledLink to={`/ChattingPage/${v.id}`}>

<FriendState props={v} where={'ChatList'}/>

</StyledLink>

);

}

위랑 같죠? 심지어 여긴 StyledLink 가 아니라 Link를 써서 검색을 하면 밑줄이 나오네요.
이러한 문제 때문에 동일한 소스코드를 복붙해서 쓰는건 피해야합니다

comforest · 2021-05-08T15:14:59Z

+              <StyledLink to="/">
+                <BsFillPersonFill size="30" />
+              </StyledLink>
+            </StyledLi>
+            <StyledLi>
+              <StyledLink to="/ChatListPage">
+                <BsFillChatFill size="25" />
+              </StyledLink>
+            </StyledLi>
+            <StyledLi>
+              <StyledLink to="/SettingPage">
+                <BsThreeDots size="30" />
+              </StyledLink>
+            </StyledLi>
+          </StyledUl>


Depth가 너무 깊어져서 가독성이 많이 떨어집니다.
Component를 다른 파일로 분리하는건 어떨까요?

kather0220

승범님 코드 보면서 이해도 잘 되고 깔끔해서 제가 많이 드릴 말씀이 없네요. 덕분에 배워가는 것도 많아요. 특히 js를 굉장히 잘 써먹으시는 거 같아요. styled components에 대한 이해도 잘 하시고 계시는 거 같구요! 코멘트를 작성하면서도 제가 하는 말이 맞나 싶었어서 그냥 느낀점이라고 생각해주시면 좋을거 같습니다.미션 하시느라 수고하셨습니다!

kather0220 · 2021-05-08T17:04:24Z

+    border: 0;
+    border-radius: 20px;
+    margin-right: 5px;
+`


styled components 를 export 해주셨네요! 그런데 다른 파일에서는 이것들을 안 쓰는 거 같은데 굳이 export 해주신 이유가 있으신가요? 아니면 그냥 별 의미 없이 하신 걸까요? 이유가 궁금합니다.

kather0220 · 2021-05-08T17:13:06Z

+                        </StyledLink>
+                    );
+                } else {
+                    if (v.name.includes(text)){


승범님도 includes를 통해서 검색 기능을 구현하셨네요! 저도 이렇게 했는데, 이 방법이 영어로 검색할 때는 대문자까지 그대로 검색해야만 되는 거라 원래의 메신저 검색 기능을 온전히 구현하지 못하는 것 같아요. 그래서 이런 식으로 바꿔주면 어떨까요? 저도 수정하려고 했는데 깜빡한 부분이라 아쉬움이 남아서 코멘트 남겨요.

Suggested change

if (v.name.includes(text)){

if (v.name.toLowerCase().includes(text)){

정말 세심한 포인트네요!

kather0220 · 2021-05-08T17:21:22Z

+
+const FriendState = ({props, where}) => {
+    const {id, name, message, img} = props;
+    const lastChat = ChatData[id][ChatData[id].length-1].text;


마지막 메시지를 표시하는 방법 저두 이렇게 시도해보려구 했는데 제 코드가 그럴 상황이 안돼서 시도를 못했거든요. 역시 승범님은 잘 구현하셨네요!!

kather0220 · 2021-05-08T17:23:28Z

+        {"isMe": true, "text": "My professor just uploaded a new assignment, and the due is tomorrow"},
+        {"isMe": false, "text": "보라색과 한 마지막 대화"}
+    ]
+}


json 파일이 깔끔하게 정리된 거 같아요! 그만큼 승범님이 코드를 효율적으로 작성하셨다는 거 같아요.

kather0220 · 2021-05-08T17:26:03Z

+    list-style: none;
+    padding: 15px;
+`;
+


styled components 들이 비교적으로 간결하게 작성된 거 같아요. 저두 styled components를 좀 더 잘 이해하면 이렇게 할 수 있을텐데..ㅎㅎ 복잡하지 않아서 보기도 편하네요. 한 수 배워갑니다!

sebastianrcnt

승범님 안녕하세요 ㅎㅎ

깔끔하게 잘 짜 주셨기도 했고, 호연님과 소정님이 리뷰를 훌륭하게 해주셔서 제가 특별히 해드릴게 없네요. 제가 늘 강조하는 부분인 네이밍과 중복 방지, 코드의 일관성만 계속 고민해보시면 더 좋은 코드가 될 수 있을 것 같습니다.

특히 최적화 해주신 부분이 또 인상적이네요 ㅎㅎ

오늘 특히 스터디에서 그림 그려서 설명하시는 부분이 정말 인상적이었습니다 ㅎㅎ

sebastianrcnt · 2021-05-09T05:16:59Z

+  // border: solid;
+  // border-width: 1px;


주석은 지워줍시당~!

sebastianrcnt · 2021-05-09T05:17:14Z

+const StyledLink = styled(Link)`
+  text-decoration: none;
+  color: black;
+`;


styled-components 상속을 정말 잘 사용해주셨네요~ㅎㅎ

sebastianrcnt · 2021-05-09T05:18:25Z

+    <GlobalStyle/>
+     <AppContainer>
+        <AppLayout>
+          <StyledUl>


StyledUl 보다는 좀 더 컴포넌트의 역할을 잘 드러낼 수 있는 네이밍이었으면 좋겠네용. Styled가 접두어로 붙은 모든 컴포넌트들도 마찬가지로요 ㅎㅎ

sebastianrcnt · 2021-05-09T05:18:43Z

+        <div>
+          <Switch>
+            <Route exact path="/" component={FriendListPage} />
+            <Route path={`/ChattingPage/:id`} component={ChattingPage} />
+            <Route path="/ChatListPage" component={ChatListPage} />
+            <Route path="/SettingPage" component={SettingPage} />
+          </Switch>
+        </div>


굳이 div를 넣을 이유가 있을까요?

sebastianrcnt · 2021-05-09T05:19:12Z

+    border-radius: 50%;
+`
+
+const Chat = memo(({ chatting, id }) => {


최적화를 잘 해주셨네요 ㅎㅎ

sebastianrcnt · 2021-05-09T05:28:49Z

+    const onChangeText = (e) => {
+        setText(e.target.value)
+    };
+
+    const onSubmitHandler = (e) => {
+        e.preventDefault();
+        if (text==='') {


Suggested change

const onChangeText = (e) => {

setText(e.target.value)

};

const onSubmitHandler = (e) => {

e.preventDefault();

if (text==='') {

const handleTextChange = (e) => {

setText(e.target.value)

};

const handleFormSubmit = (e) => {

e.preventDefault();

if (text==='') {

sebastianrcnt · 2021-05-09T05:29:42Z

+        <StyledForm onSubmit={onSubmitHandler}>
+            <Input placeholder="Message..." value={text} onChange={onChangeText} />
+            <Button>전송</Button>
+        </StyledForm>


Suggested change

<StyledForm onSubmit={onSubmitHandler}>

<Input placeholder="Message..." value={text} onChange={onChangeText} />

<Button>전송</Button>

</StyledForm>

<Wrapper onSubmit={onSubmitHandler}>

<Input placeholder="Message..." value={text} onChange={onChangeText} />

<Button>전송</Button>

</Wrapper>

한 파일 안에 하나의 컴포넌트가 있다면, 그 컴포넌트의 styled-component는 Wrapper로 네이밍하는 경우가 많더라구요~!

sebastianrcnt · 2021-05-09T05:32:14Z

+        </Top>
+        <Input placeholder=" 이름 검색" value={text} onChange={onChangeText} />
+        {friendList.map((v) => {
+            if (text === ''){


Suggested change

if (text === ''){

if (!text){

if 문 내부의 값은 boolean으로 자동으로 귀결됩니다. 요렇게만 해도 괜찮습니다 ㅎㅎ

sebastianrcnt · 2021-05-09T05:33:11Z

+    font-size: 12px;
+`;
+
+const FriendState = ({props, where}) => {


흠.. props를 이렇게 가져오는 게 맞나요?
where도 좀 더 구체적으로 네이밍 했으면 좋을 것 같네요 ㅎㅎ

sebastianrcnt · 2021-05-09T05:35:06Z

+const H3 = styled.h3`
+    padding-left: 10px;
+`;
+const Ver = styled.div`


Suggested change

const Ver = styled.div`

const VersionInfo = styled.div`

이렇게 좀 더 구체적으로?!

sbl133 added 17 commits March 23, 2021 22:16

chore: setting

e0ab0d4

feat: TopButton

1d13bca

feat: Chat

fd3dd2c

feat: Form

6dd55ad

feat: scrollToBottom

3c9740b

feat: overall css

3623ea7

refactor

ca52ae6

feat: add FreindListPage

6e2cc4b

feat: add ChatListPage

b2f3503

feat: add SettingPage

770b833

feat: react-route (friendlist, chatlist, setting)

f500242

feat: react-route (ChattingPage)

3a8098f

feat: express lastChat in ChatListPage

8b3ed8c

feat: overall css

4bbb860

refactor

d96a9d4

refactor2

79ac401

style

6b6965c

comforest reviewed May 8, 2021

View reviewed changes

kather0220 reviewed May 8, 2021

View reviewed changes

sebastianrcnt reviewed May 9, 2021

View reviewed changes

	{"isMe": false, "text": "hey wassup You to party tonight?"},
	{"userId": 0, "text": "hey wassup You to party tonight?"},

	<BsPersonPlus size="24" padding-right="10"/>
	<BsPersonPlus size="24" padding-right="10"/>

	{where === 'FriendList' ? <Message>{message}</Message> : <Message>{lastChat}</Message>}
	{<Message>{where === 'FriendList' ? message : lastChat}</Message>}

		{isMe ? <TopIcon src="/img/white.png" alt="image" /> :
		<TopIcon src={`/img/${id}.png`} alt="image" />}

	{isMe ? <TopIcon src="/img/white.png" alt="image" /> :
	<TopIcon src={`/img/${id}.png`} alt="image" />}
	<TopIcon src={isMe ? "/img/white.png" : `/img/${id}.png`} alt="image" />

	if (v.name.includes(text)){
	if (v.name.toLowerCase().includes(text)){

Conversation

sbl133 commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comforest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kather0220 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastianrcnt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbl133 commented Apr 28, 2021 •

edited

Loading

sebastianrcnt left a comment •

edited

Loading