[3주차] 이호연 과제 제출합니다. #16
Conversation
| <Route | ||
| path="/user" | ||
| render={(props) => <User {...props} userList={userList} />} | ||
| /> |
There was a problem hiding this comment.
| <Route | |
| path="/user" | |
| render={(props) => <User {...props} userList={userList} />} | |
| /> | |
| <Route | |
| path="/user" | |
| > | |
| <User userList={userList} /> | |
| </Route> |
혹시 이렇게 해도 작동 하던가요??
이렇게 하면 좀 더 직관적이고 좋을 것 같다는 생각이 들어용
prop으로 익명함수를 전달하는 것보다 children으로 넣어주는 패턴이 리액트의 설계의도와도 잘 맞을 듯 합니다 ㅎㅎ
| const list = props.chatData.map((item, index) => { | ||
| return ( | ||
| <ChatItemContainer |
There was a problem hiding this comment.
list보다는 좀 더 구체적인 이름이면 좋을 것 같아요. ChatItemList 도 괜찮아보이네용.
(대문자인 이유는 JSX 컴포넌트들의 리스트라는 걸 암시)
| const list = props.chatData.map((item, index) => { | |
| return ( | |
| <ChatItemContainer | |
| const ChatItemList = props.chatData.map((chatItem, index) => { | |
| return ( | |
| <ChatItemContainer |
| const list = props.chatData.map((item, index) => { | ||
| return ( | ||
| <ChatItemContainer | ||
| chatList={item.chatList} | ||
| chatList={item.chat} | ||
| user={item.user} | ||
| ownerUserId={props.ownerUserId} | ||
| ref={index === props.chatData.length - 1 ? lastItem : undefined} |
There was a problem hiding this comment.
요 부분에서
index === props.chatData.length - 1을 쓰는것보다
그 위에서
const isLatestChat = index === props.chatData.length - 1;이렇게 해보는건 어떨까요? 그러면 이 줄을
ref={isLatestChat ? lastItem : undefined}이렇게 바꿔볼 수 있어서 가독성도 올라갈것같네요~!
| return; | ||
| } | ||
|
|
||
| const lastChatItem = chatData[chatData.length - 1]; |
| const StyledContainer = styled.div` | ||
| display: flex; | ||
| flex-direction: row; | ||
| margin: 10px 0; | ||
| `; | ||
|
|
||
| const StyledLabelContainer = styled.div` | ||
| display: flex; | ||
| flex-direction: column; | ||
| `; | ||
|
|
||
| const StyledProfileImage = styled.img` | ||
| width: 70px; | ||
| height: 70px; | ||
| `; | ||
| const StyledText = styled.p` | ||
| margin: 5px 0; | ||
| `; |
There was a problem hiding this comment.
보통은 컴포넌트 파일 작성 시 컴포넌트를 최상위에 작성하고, styled-components를 그 밑에서부터 배열하는 방식으로 작성하는 경우가 많더라구요. 컴포넌트 파일을 열었을 때 컴포넌트 객체가 가장 위에 있으면 읽거나 찾기도 쉽겠죠?
| const myChatList = []; | ||
| for (const chatData of chatList) { | ||
| for (const userData of chatData.userList) { | ||
| if (userData.id === 0) { | ||
| myChatList.push(chatData); | ||
| break; | ||
| } | ||
| } | ||
| } |
|
|
||
| useEffect(() => { | ||
| const filteredList = | ||
| searchText === '' |
There was a problem hiding this comment.
| searchText === '' | |
| !searchText |
|
|
||
| export default ({ chatList }) => { | ||
| return ( | ||
| <Fragment> |
There was a problem hiding this comment.
<></>가 사실은
<React.Fragment></React.Fragment>이더라구요?
<></> 라고 줄여써보는것도 좋은것같아용
| const StyledContainer = styled.div` | ||
| display: flex; | ||
| flex-direction: column; | ||
| height: 100vh; | ||
| background: #ececed; | ||
| `; | ||
|
|
||
| const StyledList = styled.ul` | ||
| padding: 0 20px; | ||
| `; | ||
|
|
||
| const StyledListItem = styled.li` | ||
| list-style: none; | ||
| margin: 10px 0; | ||
| `; | ||
|
|
||
| const StyledListItemImage = styled.img` | ||
| width: 40px; | ||
| height: 40px; | ||
| padding: 10px; | ||
| `; |
| const [componentList, setComponentList] = useState([]); | ||
|
|
||
| const friendList = []; | ||
| for (const userData of userList) { | ||
| if (userData.id !== 0) { | ||
| friendList.push(userData); | ||
| } | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| const filteredList = | ||
| searchText === '' | ||
| ? friendList | ||
| : friendList.filter((item) => { | ||
| if (searchText === '') return true; | ||
| if (item.name.includes(searchText)) return true; | ||
|
|
||
| return false; | ||
| }); | ||
|
|
||
| setComponentList( | ||
| filteredList.map((item, index) => { | ||
| return <UserListItem user={item} />; | ||
| }) | ||
| ); | ||
| }, [searchText]); |
There was a problem hiding this comment.
이렇게 component를 state에 넣어두는게 일반적인 패턴인지는 잘 모르겠어용.
state가 원래 view와 data를 분리하기 위해서 만들어진건데, state에 렌더링 로직(컴포넌트)을 넣는 게 리액트의 설계의도와 좀 다른 것 같아서, 아예 컴포넌트를 분리해보는게 어떨까 싶어요 ㅎㅎ
파일 내에 컴포넌트 여러개를 선언할 수 있고, 이 파일 내에서만 쓰는 컴포넌트라면 동일한 파일 내에 세부 컴포넌트를 여러 개 작성해도 좋을 것 같아요.
호연님 코드 전반적으로 JSX 컴포넌트를 변수에 저장해서 좀 더 깔끔하게 코드를 작성하려고 하는 느낌이 들어요. 조금 더 state와 view를 나눠서 코딩 할 수 있다면 좋을 것 같아요.
| /> | ||
| </Switch> | ||
|
|
||
| <Switch> |
There was a problem hiding this comment.
웹페이지를 처음 시작할때 안에 내용이 없고 레이아웃만 뜨던데
제 생각에는 아마 여기서 path = "/" 일때 반환하는 component가 없어서 그런거 같아요
웹페이지가 처음 시작할때 path가 "/"인것을 감안해서 그 경우에도 반환하는 component가 있을면 좋을거 같습니다
|
|
||
| export default ({ chatList }) => { | ||
| return ( | ||
| <Fragment> |
There was a problem hiding this comment.
제가 알기론 Fragment를 <> </>같은 단축문법으로 쓰면 더 간단하고 빠르게 쓸수 있던데
그렇게 안하신 이유가 따로 있을까요?
| <StyledListItem> | ||
| <Link to="/user"> | ||
| <StyledListItemImage | ||
| src={`${process.env.PUBLIC_URL}/ic_person.png`} |
There was a problem hiding this comment.
혹시 아이콘이미지를 따로 저장하고 부르신거라면 react-icons라는 라이브러리를 사용해 보시는것도 좋을거같아요
react-icons가 아이콘이미지(?)같은거를 구현하기 훨씬 쉽게 해주더라고요
| <StyledSearchForm> | ||
| <StyledSearchInput value={searchText} onChange={onSearchTextChanged} /> | ||
| </StyledSearchForm> | ||
| {chatItemComponents} |
There was a problem hiding this comment.
저는 검색기능같은 경우 return문에서 map과 includes 등을 이용해서 구현했었는데 그렇게 하면
return문의 가독성이 떨어지는 거 같았거든요 그런데 호연님은 useeffect로 구현하셨더라구요
호연님의 방법으로 하면 비교적 코드가 더 깔끔해 보이는거 같아서 이런 방법도 있다는것을 알아갑니다
seank200
left a comment
seank200
left a comment
There was a problem hiding this comment.
안 그래도 지적할 것 없는 코드에 나머지 분들이 제가 하고 싶은 말을 이미 거의 다 달아주셔서 정말 눈에 불을 켜고 간신히 찾아낸 것들을 코멘트에 적었는데, 적고 보니 너무 사소한 것들을 트집 잡는 것 같아 마음이 좋지 않네요. 같은 코드도 길이를 짧게 불필요한 부분 없이 쓰는 테크닉은 항상 정말 많이 배우고 가서 매주 코드리뷰 하고 싶다는 생각이 드는 코드입니다. 다만 Web 보다는 Java와 Android 출신의 흔적이 여기저기서 보이는데, 워낙 코딩 마스터셔서 금방 고치실 것 같습니다 바쁜 와중에 수고많으셨습니다 👏
| <Switch> | ||
| <Route | ||
| path="/user" | ||
| render={(props) => <User {...props} userList={userList} />} |
There was a problem hiding this comment.
일반적으로 Component의 이름과 파일 이름을 동일하게 설정하는 편인데 User/index.js에 있다고 설명이 있으면 좋을 듯 합니다
| }, [searchText]); | ||
|
|
||
| const onSearchTextChanged = (e) => { | ||
| console.log(`_${e.target.value}_`); |
| setChatItemComponents( | ||
| filteredList.map((item, index) => { | ||
| return ( | ||
| <Link to={`Chat/${index}`}> |
There was a problem hiding this comment.
일반적으로 url path는 소문자로 하는 것으로 알고 있는데 특별한 이유가 있는지 궁금합니다
| ); | ||
| }, [searchText]); | ||
|
|
||
| const onSearchTextChanged = (e) => { |
There was a problem hiding this comment.
| const onSearchTextChanged = (e) => { | |
| const handleSearchTextChanged = (e) => { |
handler 함수이니 이렇게 하면 어떨까요?
| `; | ||
|
|
||
| const StyledSearchInput = styled.input.attrs({ | ||
| type: 'text', |
There was a problem hiding this comment.
저는 개인적으로 attribute가 React component나 HTML element하고 따로 떨어져 있으면 가독성이 떨어진다고 느껴서 이 기능을 잘 사용하지 않는데, 로직에 큰 영향을 미치지 않은 속성들은 빼도 나쁘지 않은 방법인 것 같네요
|
|
||
| return ( | ||
| <StyledContainer> | ||
| <Header user={currentUser} onClickImage={toggleCurrentUser} /> |
There was a problem hiding this comment.
뒤로 가기를 누르면 채팅 목록으로 돌아갈 수 있지만, 채팅 목록으로 돌아가는 UI가 하나 있어도 좋을 것 같아요
| `; | ||
|
|
||
| export default ({ datas }) => { | ||
| const { id } = useParams(); |
There was a problem hiding this comment.
user의 id도 있고, 이 경우 채팅방의 id를 뜻하는 것이어서, userId 또는 roomId 같이 id가 구분지어졌다면 더 좋았을 것 같아요. 물론 namespace가 있어서 코드 동작에는 문제가 없겠지만, 나중에 코드가 좀더 복잡해지는 상황을 가정했을 때, 코드를 이해하고 관리하기 더 쉬워질 것 같다는 생각이 듭니다
4 participants
과제 제출 늦어서 죄송합니다....
다들 바쁘시겠지만 여러 과제가 겹처 설정부분을 만들진 못했습니다.
기본 기능으로 주어진 검색, 라우팅 기능을 이용해서 유저 화면 체팅 화면을 구현했습니다.
SSR 에 대해서 공부하라 해서 보니 Next 라는 것도 공부해 보긴 했는데 막상 하기엔 너무 어려워서 일단 react-dom-router를 이용하였습니다. SEO 등을 위해서 Next.js 를 공부해보는 시간을 가지는 것도 나쁘지는 않을 것 같네요.
기간이 길어 추가적인 기능을 많이 넣어보고 싶었는데 커녕 전부 완성도 하지 못해 죄송합니다... ㅠㅜㅠㅜ 담주는 열심히 할게요