-
Notifications
You must be signed in to change notification settings - Fork 9
[3주차] 안건희 미션 제출합니다 #14
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
yehey-1030
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.
코드 전반적으로 매우매우 깔끔해서 읽는것도 편했고 기능도 다 잘 구현된 것 같아요! 저는 아직 채팅 저장하는 기능을 안넣어서 건희님 코드 참고하면서 기능 추가해보려고 합니다! 좋은코드 감사해용~핱튜
| }, | ||
| ]; | ||
|
|
||
| export const chatData = [ |
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.
여기도 채팅...대박....저는 귀찮..아서 채팅을 다 안저장햇는데..해야겠어요ㅠㅠ
| // export default function ChatList(props) { | ||
| // const { chatData } = props; | ||
| // const containerRef = useRef(null); | ||
| // const list = chatData.map((item) => { | ||
| // return <MessageItem chatItem={item} />; | ||
| // }); | ||
|
|
||
| // useEffect(() => { | ||
| // containerRef.current.scrollTo(0, containerRef.current.scrollHeight); | ||
| // }, [list]); | ||
|
|
||
| // return <Container ref={containerRef}>{list}</Container>; | ||
| // } | ||
|
|
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.
주석은 지우는게 가독성에 더 좋을 것 같아요!
| useEffect(() => { | ||
| // console.log(chatListRef); | ||
| // chatListRef.current.scrollBy(0, 1000) | ||
| }); |
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.
아무 것도 안하는 것처럼 보이는데 맞다면 이것도 지우는게 좋을 것 같아요!
| height: 20px; | ||
| margin-left: 30px; | ||
| `; | ||
| export default function InfoScreen(props) { |
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 를 계속 전달전달해서 그런지 저는 뭘 전달하는지 잘 모르겠...어요. 아마 제가 짠 코드가 아니라서 더 그런 것 같아요! props.list가 뭔지 주석으로 달아주시면 더 이해가 쉬울 것 같아요!
| /> */} | ||
| {list.map((item) => { | ||
| return ( | ||
| <Link to={`/chatlist/${item.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.
선종님 코드에서 봤던 ${} 이네요! 이건 무슨 뜻인가요??!
| @@ -0,0 +1,10 @@ | |||
| import styled from 'styled-components'; | |||
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 toggleVisibleSearch() { | ||
| setIsVisibleSearch(!isVisibleSearch); | ||
| } |
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.
검색창 보였다 안보였다 하는거 좋은 아이디어 같아요! 완전 귀엽네요!
seank200
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.
코딩하느라 고생 많으셨습니다 전반적으로 코드가 군더더기 없이 깔끔했고, naming convention의 통일, 불필요한 주석 정리 등만 하면 좋을 것 같아요.
| <Route path={['/', '/chatlist', '/setting']} component={MenuBar} /> | ||
|
|
||
| <Switch> | ||
| <Route exact path="/" component={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.
나중에 다른 홈페이지/랜딩페이지 추가를 대비해서 기본 path (/)는 랜딩페이지로, 친구목록은 다른 route를 써서 사용했으면 좋았을 것 같아요!
| flex-direction: column; | ||
| `; | ||
|
|
||
| // const chatData = [ |
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 ( | ||
| <Container> | ||
| <Profile userData={user} clickProfile={clickProfile} /> |
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.
| <Profile userData={user} clickProfile={clickProfile} /> | |
| <Profile userData={user} onClickProfile={handleClickProfile} /> |
prop 이름과 function 이름이 같은데, 이벤트를 처리하는 attribute의 이름은 on[이벤트 이름], 그 이벤트를 처리하는 핸들러(handler) 함수는 handle[이벤트 이름]으로 처리해도 좋을 듯 합니다
| var time = now.getHours() + ':' + minutes; | ||
|
|
||
| // eslint-disable-next-line import/no-anonymous-default-export | ||
| export default function (props) { |
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.
| export default function (props) { | |
| export default function Profile(props) { |
익명함수를 default export 하는 것은 권장하지 않고 있습니다. 그래서 윗줄에서 comment를 써서 eslint가 warning을 발생시키는 것을 임시로 막아둔 것 같은데 익명함수가 아닌 함수 이름 지정 export 하면 좋을 것 같아요!
필요한 최소한의 기능은 모두 구현해두었고, 풍부하지는 않아도 깔끔하게 css 작성했습니다. css에 생각만큼 시간을 쏟지 못한게 아쉽습니다.
왜인지 모르게 채팅방에서 이미지 엑박이 뜨네요...수정 후 다시 링크 걸어두겠습니다.
https://react-messenger-13th2.vercel.app/