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

[4주차] 이은비 미션 제출합니다 #22

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

silverain02
Copy link

배포링크

배포링크

후기

디자이너님이 전달주신 QA에서 버튼 활성화,비활성화에 따라 크기가 달라지는 문제가 있어서 일부 수정하였고, 채팅방 내 날짜 표시 기준이나 채팅의 시간 표시 간격 등 논의해 디자인도 수정했습니다~

이전 주차에서 한 채팅방과 연결이 안되는 오류가 있어 해결 중에 있습니다 ㅜㅜ 변명이지만 시험이 늦게 끝나서 깔끔하게 완성을 못한 것 같네요..
최대한 스터디 전에 해결해보겠습니다!!

다들 고생하셨어용

Key Questions

  • Page Routing
    리액트의 SPA 특징을 살리기 위해 최대한 페이지를 최소화하고 한 페이지 내에서 컴포넌트를 교체하는 방식을 사용했습니다.
  • State관리
    Redux를 사용하려고 공부해봤는데 이번 주차 미션같이 Props가 많지 않는 프로젝트의 경우 전역 상태관리가 크게 필요없을 것 같아 적용하지는 않았습니다. 다음에는 Redux나 Recoil을 좀 더 공부해서 적용해보고 싶습니다~!

Copy link

@rmdnps10 rmdnps10 left a comment

Choose a reason for hiding this comment

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

이번 메신저 과제 구현하느라 정말 수고 많으셨습니다~~!

코드 리뷰하면서 많이 배웠습니다:)

</Link>
<Link to="/mypage">
<MyPageIcon
fillColor={location.pathname === '/mypage' ? '#9747ff' : '#d9d9d9'}
Copy link

Choose a reason for hiding this comment

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

저는 아이콘 활성화 시킬 때 아이콘 활성화 여부를 상태로 관리하여 useEffect 구문에서 location 주소가 바뀜에 따라 상태를 설정해주었는데, 상태관리 없이 이렇게 쓸 수도 있겠네요~!

Comment on lines 40 to 42
width: 100%;
height: 100%;

Copy link

Choose a reason for hiding this comment

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

PC 화면상으로 UI가 깨지는데, 최상위 컴포넌트에 고정된 width와 height를 설정하고 margin: 0 auto 를 통해 해결할 수 있을 거 같습니다!

Comment on lines +23 to +25
fetch('data/userData.json')
.then((res) => res.json())
.then((result) => setUserData(result));
Copy link

Choose a reason for hiding this comment

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

실제 백엔드와 통신하는 것처럼 json 파일을 fetch하셨군요 ㅎㅎ 멋있습니다!!

Comment on lines +8 to +23
const ChatIcon: React.FC<IconProps> = ({ fillColor = '#d9d9d9' }) => {
return (
<svg
width="64"
height="52"
viewBox="0 0 64 52"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<path
fill-rule="evenodd"
clip-rule="evenodd"
d="M31.9999 33.6569C38.1041 33.6569 43.0525 29.6981 43.0525 24.8148C43.0525 19.9314 38.1041 15.9727 31.9999 15.9727C25.8957 15.9727 20.9473 19.9314 20.9473 24.8148C20.9473 27.7076 22.6837 30.2759 25.3683 31.8891V35.1395C25.3683 35.8797 26.2236 36.2923 26.803 35.8315L29.7642 33.4759C30.4863 33.5945 31.234 33.6569 31.9999 33.6569Z"
fill={fillColor}
/>
</svg>
Copy link

Choose a reason for hiding this comment

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

svg 파일을 컴포넌트로써 선언하시고 색깔을 props을 전달하여서 fill 속성에 넣어줘 이미지 색상을 구현하셨군요~! svg를 컴포넌트화 시키면 다양한 응용이 있을 것 같습니다, 배워갑니다!

@@ -77,7 +86,7 @@ const RightWrapper = styled.div`
justify-content: space-between;
align-items: center;
width: 80%;
gap: 5px;
gap: 7px;
`;

const Wrapper = styled.div`
Copy link

Choose a reason for hiding this comment

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

개인적인 생각인데, styled-component 이름을 명명함에 있어서 Wrapper 보다는 좀 더 구체적인 명명이 좋지 않을까 생각합니다! 만약 page별로 동일한 Wrapper를 적용한다면, 최상위 컴포넌트에 동일한 스타일을 지정해줄 수도 있을 거 같아용

Comment on lines +85 to +98
useEffect(() => {
const currentDate = getCurrentDate();
if (inputText.trim() !== '') {
const newItem: ChatData = {
userId: 0,
isLike: false,
chat: inputText,
time: currentDate,
};
const updatedChatData = [...chatData, newItem];
setChatData(updatedChatData);
console.log(updatedChatData);
}
}, [inputText]);
Copy link

Choose a reason for hiding this comment

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

useEffect 훅 안에서 inputText가 변화할 때마다 updatedChatData를 변경하는 것보다는 onChange를 통해 따로 관리한 inputTexthandlesubmit 함수가 실행될 때에만 updatedChatdata에 업데이트해주는 건 어떨까요?

Comment on lines +40 to +46
const newText = inputRef.current.value.trim(); // 입력값에서 앞뒤 공백을 제거
if (newText) {
setInputText(newText); // 유효한 입력값을 상위 컴포넌트로 업데이트
inputRef.current.value = ''; // 입력 필드 초기화
} else {
// 유효하지 않은 입력값에 대한 처리를 추가할 수 있습니다.
console.log('유효하지 않은 입력값');
Copy link

Choose a reason for hiding this comment

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

onChange 이벤트헨들러로 inputText를 관리한다면, inputRef로 접근할 필요없이 기존의 상태관리 변수인 inputText를 이용해도 될 것 같아요!

Comment on lines +97 to +101
useEffect(() => {
if (isInputFocused && bodyRef.current) {
bodyRef.current.scrollTop = bodyRef.current.scrollHeight;
}
}, [isInputFocused]);
Copy link

Choose a reason for hiding this comment

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

현재 메시지를 보낸 후에 스크롤바가 자동으로 내려가지 않고 있는데, useEffect 안의 의존성 배열을 isInputFocusd가 아닌 메시지 리스트로 바꾸면 해결될 거 같아요!

2023-11-04.6.43.29.mov

Comment on lines +112 to +120
time={
(index === 0 || chatData[index - 1].time !== chatItem.time) &&
!(
index < chatData.length - 1 &&
chatData[index + 1].time === chatItem.time
)
? chatItem.time
: ''
}
Copy link

Choose a reason for hiding this comment

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

시간 설정의 경우에 훅으로 따로 빼주면 더 코드의 가독성이 더 좋아질 거 같습니다!

if (inputRef.current) {
const newText = inputRef.current.value.trim(); // 입력값에서 앞뒤 공백을 제거
if (newText) {
setInputText(newText); // 유효한 입력값을 상위 컴포넌트로 업데이트
Copy link

Choose a reason for hiding this comment

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

현재 메시지를 전송할 때 setInputText 를 통해 inputText의 상태를 업데이트하고, useEffect 안에서 inputText를 의존성 배열로 설정하여서, 전송했을 때 inputText의 상태에 따라 updatedChatdata를 갱신하고 있는데 이 경우 전송 하기 전 마지막으로 보낸 메시지와 내가 보려는 메시지가 같을 경우 상태 변경으로 인식하지 못하여 보내지지 않는 버그가 일어나는 것 같습니다!

2023-11-04.6.46.08.mov

Copy link

@geeoneee geeoneee left a comment

Choose a reason for hiding this comment

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

리뷰하면서 많이 배우고 가요!! 과제 수고 많으셨습니다:)

fill={fillColor}
/>
</svg>
);
Copy link

Choose a reason for hiding this comment

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

Icon들이 많아서 따로 Icon 컴포넌트로 분리해서 사용하시는건 어떨까요?
코드가 중복되는 부분을 줄일 수 있을 것 같아요! 아이콘 컴포넌트

title="Instagram"
linkTo="https://instagram.com/silverain02_?igshid=YTQwZjQ0NmI0OA%3D%3D&utm_source=qr"
/>
<ProfileLink
Copy link

Choose a reason for hiding this comment

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

마이페이지 아이콘들도 svg 사용하면 화질 깨짐을 방지할 수 있을 것 같아요!

<SettingIcon fillColor="black" />
</IconList>
</MyHeader>

Copy link

Choose a reason for hiding this comment

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

Header 부분이 chatlist, chatroom, friendliest, mypage에 모두 있는 것 같아서 이 부분을 컴포넌트로 만들어서 한 번에 관리해도 좋을 것 같아요!

)
? chatItem.time
: ''
}
Copy link

Choose a reason for hiding this comment

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

스크린샷 2023-11-05 오후 1 19 57 같은 시간에 채팅을 보내면 시간이 표시가 안되는 것 같습니다!


margin-top: 15px;

cursor: pointer;
Copy link

Choose a reason for hiding this comment

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

cursor 부분을 backIcon에만 주는게 사용자가 사용하기 더 편리할 것 같아요:)

const [userName, setUserName] = useState('');

const [inputText, setInputText] = useState('');
const [isInputFocused, setInputFocused] = useState(false);
Copy link

Choose a reason for hiding this comment

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

입력할 때 focused 준 디테일 좋은 것 같아요:)

.then((result) => {
setUserData(result);
});
}, []);
Copy link

Choose a reason for hiding this comment

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

fetch를 사용하셔서 나중에 백 쪽과 연결할 때 편리할 것 같습니다:)

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.

3 participants