-
Notifications
You must be signed in to change notification settings - Fork 5
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
[Review] ? 찍기, 건배 애니메이션 구현 및 폰트요청 버그수정 #71
Conversation
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.
안녕하세요 늦은 리뷰어 왔습니다ㅎㅎ..
보면서 친숙한 소켓 코드가 많은데도 구현 방식이 많이 달라서 굉장히 새롭고 신기했습니다! 😁
코드를 열심히 읽긴 했지만 100% 이해한 건 아닌 것 같아
리뷰 중 이상하게 이해한 부분이 있다면 언제든 말씀해주시면 다시 보겠습니다 🙃
이번주도 수고 많으셨습니다!!
const QuestionMark: React.FC<QuestionMarkPropTypes> = ({ identifier, disappearSelf, x, y }) => { | ||
useEffect(() => { | ||
setTimeout(() => { | ||
disappearSelf(identifier); | ||
}, 1900); | ||
}, []); | ||
return <QuestionImg x={x} y={y} />; | ||
}; |
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.
와 물음표 애니메이션 어떻게 구현하실지 굉장히 궁금했는데 😃
생성 이후 일정 시간이 지나면 자신을 사리지게하는 (제가 이해한 게 맞겠죠?) 컴포넌트로 만들어서
상태값과 연결시키셨군요! 참신한 발상이네요 😮😮
다만 이런 시간 같은 값은 컴포넌트 밖에서 상수처럼 사용하시는게 보기와 사용에도 좋을 것 같아요!!
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(() => { | ||
const functions = Socket.animation({ setIsCheers }); | ||
activateCheers.current = functions.activateCheers; | ||
return () => { | ||
functions.disconnecting(); | ||
}; | ||
}, []); |
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.
Socket에서 event를 emit하거나 설정하는 함수를 반환하는 것으로 보이네요!
Socket.animation({ setIsCheers })
과 같이 호출하는 방식이 굉장히 신기하네요 😮
(어떻게 돌아가는 건지는 코드를 좀 더 봐야겠어요ㅎㅎ)
functions
로 묶는 이유가 특별하게 있는 것이 아니시라면 이런식으로 사용하는 것도 좋아보여요! 🙂
useEffect(() => {
const { activateCheers, disconnecting } = Socket.animation({ setIsCheers });
activateCheers.current = activateCheers;
return () => {
disconnecting();
};
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.
Socket
이라는 Typescript 파일을 하나 만들어서, 각 역할별로 별도의 메서드를 만들어주고 그 안에서 각각 다르게 처리하는 방식으로 구현하였습니다!
더 자세하게 설명하기 위해 사진을 가져와봤습니다!
아래 파일은 순서대로 client/src/socket/socket.ts
파일과 client/src/socket/animation.ts
인데요! 여기서 공통으로 사용하는 connect
, disconnect
등을 따로 처리하고, 기능별로 따로 사용하는 부분은 파일을 만들어서 그 안에서 새롭게 메서드를 지정하는 방식으로 작동하고 있습니다 ㅎㅎ
그리고 말씀하신대로 구조할당분해
를 사용해서 코드를 더 간단하게 만들 수 있겠네요! 조언 감사합니다!! 🙂
import { Socket } from 'socket.io-client'; | ||
|
||
const QUESTION = 'QUESTION'; | ||
|
||
const questionmark = (socket: Socket) => (closure: any) => { | ||
const { setMarks } = closure; | ||
let count = 0; | ||
|
||
socket.on(QUESTION, ({ x, y }) => { | ||
setMarks((prev) => { | ||
count++; | ||
return { ...prev, [count]: { x, y } }; | ||
}); | ||
}); | ||
|
||
const questionMark = (mydata) => { | ||
console.log(mydata); | ||
socket.emit(QUESTION, mydata); | ||
}; | ||
|
||
const disconnecting = () => { | ||
socket.off(QUESTION); | ||
}; | ||
|
||
return { questionMark, disconnecting }; | ||
}; | ||
|
||
export default questionmark; |
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.
아하 저희의 소켓 관련 커스텀 훅과 유사한 역할을 하는 함수인 것 같네요!
소켓 이벤트로 상태값을 변경해야하는 경우 closure
를 통해 상태값 변경 함수를 받아오고 설정해주는 거군요!
개인적으로는 소켓과 관련해서 변화하는 상태값이라면 소켓을 설정할 때 내부에서 관련 상태값을 함께 생성해주고
상태값이나 소켓 이벤트를 emit하는 함수만 반환해주는 형태를 선호합니다!
이유를 들자면 소켓으로 변화하는 상태값은 결국 socket.on()
내부에서 상태를 변경하게 되기 때문에
외부에서 setState
함수를 쓸 일이 거의 없고, 소켓과 연관된 상태값이 떨어져 있게 되면
결합도가 떨어지는 느낌이 들기도 했습니다. 소켓에 관련 상태값 정보를 넘겨줘야하는 부분도 번거롭기도 했구요!
(물론 개인적인 생각입니다 😅💦)
소켓 이벤트를 emit하는 함수는 잘 구현하셔서 반환하시고 계신 것 같으니 조금만 수정하셔서
커스텀 훅처럼 관련 상태값도 내부에서 가지고 있도록 해보시는 것도 좋을 것 같아요! 🤗
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.
저도 수빈님 의견에 동의합니다!ㅎㅎ
조금만 더 첨언 드리자면, 저도 리액트 공부하면서 외부에서 다른 곳의 setState
를 받아서 상태값을 변화시키는 형태는 각 컴포넌트의 결합도를 높여서 저희가 흔히 이야기하는 의존성이 높은 구조를 가지게 된다고 들었습니다!
그래서 해당 컴포넌트와 관련하여 상태값 변화가 필요하다면 setState
가 사용된 로직이 들어간 함수를 생성하고 해당 함수를 반환해주는 형태로 많이 사용한다고해요!
그리고 추가적으로 저희가 멘토님께 들었던 피드백을 전해드리면 커스텀 훅같은 경우 다른 컴포넌트에서도 자주 사용되느냐 되지 않느냐를 잘 생각해봐야한다고 하더라구요! 저희도 사실상 커스텀 훅같은 경우에 한 번씩 밖에 사용되지 않는데 코드 분리를 위해 커스텀 훅을 만들어 사용했어요...ㅎㅎ 그래서 이 부분을 지적 받았구요! 🥲 저희 같은 경우에는 추후 (라고 쓰고 언제가 될지 모르지만...) 커스텀 훅 내부에서 코드상 반복되는 형태가 존재하는데 이 형태를 따로 빼서 커스텀 훅으로 만들어보려해요!
정확한 구조는 제가 몰라서 혹시 이 함수가 커스텀 훅처럼 사용되는 함수가 맞다면 여러곳에서 사용될 수 있는지 생각해보시면 좋을 것 같아 저희가 들었던 피드백을 슬쩍 한 번 이야기 해봅니다!! 😆
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.
요 부분이 리뷰 맛집이군요ㅎㅎ
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.
소중한 멘토 피드백까지 알려주시니 정말 감사합니다 ㅠㅠㅠ 생각해보면 setState
함수들을 사용하는 부분이 socket
인데, 실제로 구현이 되어있는 부분은 각 컴포넌트 내부이다보니 부득이하게 컴포넌트와 소켓처리파일이 연결되어 버리는 문제가 있었네요!
추후에 리팩토링할때 추가적인 문제가 발견되지 않는다면 관련 상태값들을 내부에 보관하는 방식으로 개선해야 할 필요가 있을 것 같습니다 :) 제가 커스텀 훅에 익숙하지 않아서.. 조금 더 공부하면서 리팩토링해봐야겠어요!! 정말 감사합니다!!!
let code = ''; | ||
code = chatRoomCode; | ||
const messageData = { | ||
...msg, | ||
msg, | ||
sid: socket.id, | ||
date: getTimeString(), | ||
}; | ||
console.log(messageData); | ||
socket.emit(RECEIVE_MESSAGE, messageData); | ||
socket.broadcast.emit(RECEIVE_MESSAGE, messageData); | ||
socket.to(code).emit(RECEIVE_MESSAGE, messageData); |
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.
socket.to(chatRoomCode).emit(RECEIVE_MESSAGE, messageData);
처럼 바로 넘겨줘도 괜찮을 것 같아요!
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.
그러네요! 혹시 code 변수 따로 만드신 이유가 있으신가요?
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.
제가 방별로 채팅을 구분하면서 작성한 부분인데.. 코드를 리팩토링 하는 과정에서 발견하지 못하고 넘어갔었네요..! 미처 신경쓰지 못했던 부분을 발견해주셔서 감사합니다 ☺
socket.on(QUESTION, ({ x, y, chatRoomCode, user }) => { | ||
let x1 = x; | ||
console.log(x1); | ||
console.log(x, y); | ||
let code = ''; | ||
code = chatRoomCode; | ||
socket.emit(QUESTION, { x, y }); | ||
socket.to(code).emit(QUESTION, { x, y }); | ||
}); |
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.
여기서 socket.emit
과 socket.to(code).emit
으로 같은 이벤트와 같은 데이터를 보내고 있는데
그럼 현재 socket은 현재 저 code
의 Room에 들어가 있지 않은 걸까요?
만약 들어가 있다면 io.to(code).emit(QUESTION, { x, y });
와 같이 합칠 수 있을 것 같네요!
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.
와..!! io
를 payload
에 실어서 보내줬는데 그걸 사용하지 않고 socket
을 이용해서 보내고 있었네요..! 말씀하신대로 io
만으로 처리하도록 바꾸니 정상적으로 동작합니다! 제가 io
와 socket
의 차이에 대해서 잘 모르고 짜다보니 이런 비효율적인 코드가 만들어진 것 같습니다 ㅋㅋㅋㅋㅋ
리팩토링하면서 비슷한 구조로 되어있는 부분들 다 수정해줘야겠어요! 정말 날카로운 지적 감사드립니다 👍👍👍
const randomDisplay = () => { | ||
const randomNum = Math.floor(Math.random() * CHEERS_GIF_NUM); | ||
const targetGif = LISTED_GIF[randomNum]; |
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.
random gif라니 너무 귀여워요 🥺🥺
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.
우와... 아이디어가 너무 좋네요!! 👍🏻
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.
전체적으로 코드를 읽으면서 너무 멋지다는 생각만 했네요..!
리뷰드릴 곳이 없어 제대로 안본다고 생각하면 어떡하나 걱정입니다 🤣
저희도 아직 리팩토링 못한 부분이지만 어제 크롱님의 피드백을 옮겨받아 전체적으로 매직 넘버 숨겨주면 좋지않을까 생각합니다!ㅎㅎ
이번주도 정말 많이 배웠습니다!! :) 너무 고생 많으셨어요ㅎㅎㅎ
const randomDisplay = () => { | ||
const randomNum = Math.floor(Math.random() * CHEERS_GIF_NUM); | ||
const targetGif = LISTED_GIF[randomNum]; |
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.
우와... 아이디어가 너무 좋네요!! 👍🏻
import { Socket } from 'socket.io-client'; | ||
|
||
const QUESTION = 'QUESTION'; | ||
|
||
const questionmark = (socket: Socket) => (closure: any) => { | ||
const { setMarks } = closure; | ||
let count = 0; | ||
|
||
socket.on(QUESTION, ({ x, y }) => { | ||
setMarks((prev) => { | ||
count++; | ||
return { ...prev, [count]: { x, y } }; | ||
}); | ||
}); | ||
|
||
const questionMark = (mydata) => { | ||
console.log(mydata); | ||
socket.emit(QUESTION, mydata); | ||
}; | ||
|
||
const disconnecting = () => { | ||
socket.off(QUESTION); | ||
}; | ||
|
||
return { questionMark, disconnecting }; | ||
}; | ||
|
||
export default questionmark; |
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.
저도 수빈님 의견에 동의합니다!ㅎㅎ
조금만 더 첨언 드리자면, 저도 리액트 공부하면서 외부에서 다른 곳의 setState
를 받아서 상태값을 변화시키는 형태는 각 컴포넌트의 결합도를 높여서 저희가 흔히 이야기하는 의존성이 높은 구조를 가지게 된다고 들었습니다!
그래서 해당 컴포넌트와 관련하여 상태값 변화가 필요하다면 setState
가 사용된 로직이 들어간 함수를 생성하고 해당 함수를 반환해주는 형태로 많이 사용한다고해요!
그리고 추가적으로 저희가 멘토님께 들었던 피드백을 전해드리면 커스텀 훅같은 경우 다른 컴포넌트에서도 자주 사용되느냐 되지 않느냐를 잘 생각해봐야한다고 하더라구요! 저희도 사실상 커스텀 훅같은 경우에 한 번씩 밖에 사용되지 않는데 코드 분리를 위해 커스텀 훅을 만들어 사용했어요...ㅎㅎ 그래서 이 부분을 지적 받았구요! 🥲 저희 같은 경우에는 추후 (라고 쓰고 언제가 될지 모르지만...) 커스텀 훅 내부에서 코드상 반복되는 형태가 존재하는데 이 형태를 따로 빼서 커스텀 훅으로 만들어보려해요!
정확한 구조는 제가 몰라서 혹시 이 함수가 커스텀 훅처럼 사용되는 함수가 맞다면 여러곳에서 사용될 수 있는지 생각해보시면 좋을 것 같아 저희가 들었던 피드백을 슬쩍 한 번 이야기 해봅니다!! 😆
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(() => { | ||
const functions = Socket.message({ setChatLog }); | ||
emits.current = functions; | ||
emits.current = functions.sendMessage; | ||
console.log(chatLog, 'adasd'); | ||
return () => { | ||
functions.disconnecting(); | ||
}; |
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.
Socket에서 함수를 가져와서 emits에 할당하는거 맞나요?
저희는 소켓 부분을 커스텀 훅으로 분리했는데 이런식으로 Socket 객체(?) 하나 만드는 것도 좋아보입니다!
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값에 함수를 담아서, 그 함수 내에서 이벤트를 처리하는 방식입니다 ㅎㅎ 위에 수빈님 댓글에 나름 자세하게 설명하려고 노력한 부분이 있으니 참고하시면 될 것 같습니다!!
저는 커스텀훅을 한번도 안써봐서.. 기회가 된다면 한번 써보고 싶네요 🤣🤣
{chatLog.map(({ sid, msg, date }, index) => ( | ||
<ChatItem isSelf={myID === sid} message={msg} date={date} key={index} /> |
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.
혹시 key 값을 index로 하신 특별한 이유가 있으신게 아니라면 고유한 값으로 바꿔주는게 좋을 것 같습니다!
해당 내용 간단하게 블로그에 정리해봤습니다..ㅎㅎ
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.
매번 warning뜨는게 싫어서 저런 식으로 자주 처리하곤 했는데, 이 부분도 조금 더 고민해봐야겠습니다 ㅎㅎ
p.s. github으로 만든 블로그임에도 디자인이 참 예쁘네요!! 잘 구경하고 갑니다 ☺
<Wrapper isSelf={isSelf}> | ||
<UserSection isSelf={isSelf}> |
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.
isSelf는 어떤 역할을 하는걸까요?? 궁금합니다 ㅎㅎ
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.
isSelf는 자신이 보낸 채팅인지를 확인하는 boolean
타입의 값입니다!
<Wrapper> | ||
<video | ||
className="myFace" | ||
ref={myVideoRef} |
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.
영상 채팅 구현하시는게 너무 멋집니다! ㅎㅎ
그런데 Wrapper 안의 태그들이 뭔가 video 관련 태그들 같은데 VideoContainer 라던지 해서 하나의 컴포넌트로 만드는 건 어떨까요??
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.
맞습니다! 다른 사람들의 영상을 뿌려주는 역할을 하는 video태그부분입니다 ㅎㅎ
이 부분을 더 깔끔하게 따로 분리할 수 있을지에 대한 내용은 다음주에 팀원들과 한번 의논해보도록 하겠습니다!!
{Object.values(streams).map((otherStream) => { | ||
return <OtherVideo srcObject={otherStream} />; | ||
})} |
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.
OtherVideo에 key 값 안넣어줘도 리액트가 오류 안뱉나요? 궁금합니다!
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.
import { Socket } from 'socket.io-client'; | ||
|
||
const QUESTION = 'QUESTION'; | ||
|
||
const questionmark = (socket: Socket) => (closure: any) => { | ||
const { setMarks } = closure; | ||
let count = 0; | ||
|
||
socket.on(QUESTION, ({ x, y }) => { | ||
setMarks((prev) => { | ||
count++; | ||
return { ...prev, [count]: { x, y } }; | ||
}); | ||
}); | ||
|
||
const questionMark = (mydata) => { | ||
console.log(mydata); | ||
socket.emit(QUESTION, mydata); | ||
}; | ||
|
||
const disconnecting = () => { | ||
socket.off(QUESTION); | ||
}; | ||
|
||
return { questionMark, disconnecting }; | ||
}; | ||
|
||
export default questionmark; |
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.
요 부분이 리뷰 맛집이군요ㅎㅎ
let code = ''; | ||
code = chatRoomCode; | ||
const messageData = { | ||
...msg, | ||
msg, | ||
sid: socket.id, | ||
date: getTimeString(), | ||
}; | ||
console.log(messageData); | ||
socket.emit(RECEIVE_MESSAGE, messageData); | ||
socket.broadcast.emit(RECEIVE_MESSAGE, messageData); | ||
socket.to(code).emit(RECEIVE_MESSAGE, messageData); |
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.
그러네요! 혹시 code 변수 따로 만드신 이유가 있으신가요?
@@ -38,7 +40,7 @@ const socketLoader = (server, app): any => { | |||
io.on('connection', (socket: Socket) => { | |||
console.log('socket connection!!', socket.id); | |||
|
|||
pipe(signaling, chatting, creating, entering)({ io, socket, rooms }); | |||
pipe(signaling, chatting, creating, entering, animation, questionMark)({ io, socket, rooms }); |
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.
pipe는 다시 봐도 멋집니다 ㅎㅎ
WorkList
Issue
? 찍은게 핫이슈
Video
Description
이것저것 많이 한 것 같은데 음.. 이게 맞는건가..?
간단한 소감 남겨주세요 ㅋㅋㅋㅋㅋ