-
Notifications
You must be signed in to change notification settings - Fork 0
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
[REFACTOR] 게임 시작 전 카운트 다운 애니메이션 기능 구현 #271
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.
리뷰 남깁니다~ 카운트 애니메이션이 이쁘게 잘 되었어요 !!
소스 코드가 많아 보였는데 대부분 모션 관련된 코드라 예상했던 것 보다는 리뷰가 빨리 끝났네요
const countMapper: Record<number, number> = { | ||
3: 1, | ||
2: 2, | ||
1: 3, | ||
}; |
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.
🙏 제안 🙏
mapper이면 key과 value를 이어준다는 의미일텐데 무엇과 무엇을 매칭 시키는지 감이 안 잡혀서요. 아래쪽 JSX를 확인해보니 땅콩 개수 리스트인 것 같은데
주석을 작성하거나 아예 object에서 array로 바꾸거나 하는 건 어떤가요?
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.
count에 따른 이미지 개수를 의미하는데 array로 바꾸는 건 어떤 걸 의미하는 걸까요?? count 가 3, 2, 1로 줄어들 때, length는 1, 2, 3으로 증가 해야합니다. 어떤 식으로 바꿀 수 있을까요?
일단은 변수명을 수정해서 imageCountMapper 로 바꿔보았습니다!
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 Mapper = [
{
count: 3,
image: 1
},
{
count: 2,
image: 2
},
{
count: 1,
image: 3
}]
이런 느낌도 괜찮을 것 같다고 생각했었습니다.
최종적인 판단은 마루에게 맡길게요!
if (memberInfo.isMaster) { | ||
startGameMutation.mutate(); | ||
startCountdown(); |
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.
💭 질문 💭
혹시 이렇게 되면 Countdown이 끝나기 전에 게임이 시작되지는 않나요?
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.
서버에 게임 시작 요청을 보내야 모든 사용자가 게임이 시작됨을 감지할 수 있습니다.
그래서 게임 시작 API를 호출해야 했고, 그 후 카운트다운을 시작하여 3초 뒤에 navigate 되도록 수정하였습니다.
기존에 isGameStart가 true일 때 navigate 되는 로직을 없애고 3초 뒤에 navigate 된다고 보시면 될 것 같습니당
describe('StartButtonContainer 테스트', () => { | ||
it('게임 시작 버튼을 클릭하면 카운트 다운을 시작한다.', async () => { | ||
const initializeIsMaster = (snap: MutableSnapshot) => { | ||
snap.set(memberInfoState, { memberId: 1, nickname: 'Test User', isMaster: true }); | ||
}; | ||
|
||
const user = userEvent.setup(); | ||
customRender(<StartButtonContainer />, { initializeState: initializeIsMaster }); | ||
const COUNTDOWN_LABEL_TEXT = '게임 시작 3초 전'; | ||
|
||
const button = await screen.findByRole('button', { name: '시작' }); | ||
await user.click(button); | ||
|
||
expect(screen.getByLabelText(COUNTDOWN_LABEL_TEXT)).toBeInTheDocument(); |
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 useCountdown = () => { | ||
const navigate = useNavigate(); | ||
const { roomId } = useParams(); | ||
const [isCountdownStart, setIsCountdownStart] = useState(false); | ||
|
||
const startCountdown = () => { | ||
setIsCountdownStart(true); | ||
}; | ||
|
||
const goToGame = () => { | ||
navigate(ROUTES.game(Number(roomId))); | ||
}; | ||
|
||
return { isCountdownStart, startCountdown, goToGame }; | ||
}; |
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.
🌸 칭찬 🌸
깔끔한 hook 분리 굿굿
const StartButton = ({ show, startCountdown }: StartButtonProps) => { | ||
const { memberInfo, handleGameStart } = useGameStart({ showModal: show, startCountdown }); | ||
|
||
return ( | ||
<Button | ||
text={master?.memberId === memberInfo.memberId ? '시작' : '방장이 시작해 주세요'} | ||
disabled={master?.memberId !== memberInfo.memberId} | ||
text={memberInfo.isMaster ? '시작' : '방장이 시작해 주세요'} | ||
disabled={!memberInfo.isMaster} |
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.
🌸 칭찬 🌸
API response가 바뀌었는데 예전 데이터를 기준으로 방장을 확인하고 있었는데
이제 깔끔하게 변경되었네요 ㅎㅎ
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.
포메 코멘트 답변 달았습니다~!
if (memberInfo.isMaster) { | ||
startGameMutation.mutate(); | ||
startCountdown(); |
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.
서버에 게임 시작 요청을 보내야 모든 사용자가 게임이 시작됨을 감지할 수 있습니다.
그래서 게임 시작 API를 호출해야 했고, 그 후 카운트다운을 시작하여 3초 뒤에 navigate 되도록 수정하였습니다.
기존에 isGameStart가 true일 때 navigate 되는 로직을 없애고 3초 뒤에 navigate 된다고 보시면 될 것 같습니당
const countMapper: Record<number, number> = { | ||
3: 1, | ||
2: 2, | ||
1: 3, | ||
}; |
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.
count에 따른 이미지 개수를 의미하는데 array로 바꾸는 건 어떤 걸 의미하는 걸까요?? count 가 3, 2, 1로 줄어들 때, length는 1, 2, 3으로 증가 해야합니다. 어떤 식으로 바꿀 수 있을까요?
일단은 변수명을 수정해서 imageCountMapper 로 바꿔보았습니다!
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 meta = { | ||
title: 'Countdown', | ||
component: Countdown, | ||
tags: ['!autodocs'], |
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.
💭 질문 💭
!autodocs로 설정한 이유가 있으신가요 😊 ?
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.
아하 그런 문제가 있어서 !autodocs로 설정을 했던 거군요! 좋아요 마루 !!!! 저도 종종 잘리는 경우가 있어서 고민을 했었는데 찾아보고 다른 해결 방안을 알게 되면 그때 추가로 공유해 볼게요 !!!! 🤭
|
||
const Countdown = ({ goToGame }: CountdownProps) => { | ||
const [count, setCount] = useState(START_COUNTDOWN); | ||
const timeout = useRef<NodeJS.Timeout>(); |
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.
💭 질문 💭
해당 코드에서 useRef의 타입이 number가 아니라 NodeJS.timeout인 이유가 궁금해요!
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.
친절한 설명 감사드려요 🌞🌞🌞 !!!1
)} | ||
<div css={imageContainer}> | ||
{Array.from({ length: imageCountMapper[count] }, (_, i) => i + 1).map((idx) => ( | ||
<img key={idx} src={SpinDdangkong} css={peanut(idx)} alt={`${idx}번째 카운트다운 땅콩`} /> |
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.
💭 질문 💭
imageCountMapper[count]가 undefined일 경우를 대비해서 || 0 작업을 하지 않아도 괜찮나요 ??
)} | ||
<div css={imageContainer}> | ||
{Array.from({ length: imageCountMapper[count] }, (_, i) => i + 1).map((idx) => ( | ||
<img key={idx} src={SpinDdangkong} css={peanut(idx)} alt={`${idx}번째 카운트다운 땅콩`} /> |
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.
🙏 제안 🙏
Array.from 만으로도 동작을 수행할 수 있을 것 같은데 어떻게 생각하시나용 ?
{Array.from({ length: imageCountMapper[count] || 0 }, (_, idx) => (
<img key={idx + 1} src={SpinDdangkong} css={peanut(idx + 1)} alt={`${idx + 1}번째 카운트다운 땅콩`} />
))}
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.
undefined여도 렌더링을 안하기 때문에 명시적인 처리를 안해줘도 된다고 생각했습니다! 하지만 명시적인 것도 좋네용
추가적으로 빈배열 자체를 안만들도록 분기처리를 추가해보았습니다!
{imageCountMapper[count] &&
Array.from({ length: imageCountMapper[count] }, (_, idx) => (
<img
key={idx + 1}
src={SpinDdangkong}
css={peanut(idx + 1)}
alt={`${idx + 1}번째 카운트다운 땅콩`}
/>
))}
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.
오오 좋은데요 ?!!! 🤭🤭🤭
<div css={countdownLayout}> | ||
<div css={dimmed} /> | ||
{count > 0 && ( | ||
<span css={countdown} aria-label={`게임 시작 ${count}초 전`}> |
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.
🌸 칭찬 🌸
aria 추가 너무 좋아요 ~~~~~!!!! 스크린 리더기로 읽었을 때 혹시 어떻게 나왔는지 궁금해요 ! 사실 추후에 집중 개선 작업이 들어갈 것이기 때문에 단지 질문입니다요 ....... 🙌
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.
3 2 1 카운트다운을 타이밍 맞춰서 읽기가 어려웠는데 생각해보니 바뀔때마다 읽어야하니까 aria-live="polite"를 추가했습니다!
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.
접근성 개선 폼 미쵸따이.
jest.advanceTimersByTime(COUNTDOWN_LENGTH); | ||
}); | ||
|
||
expect(goToGameMock).toHaveBeenCalled(); |
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.
🌸 칭찬 🌸
게임 화면으로 잘 넘어가는 것까지 테스트하는 꼼꼼함 .. !!!!
width: ${2.4 * idx}rem; | ||
height: ${3.6 * idx}rem; | ||
|
||
animation: ${peanutAnimation} 1s ease-in-out infinite; |
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.
추가 반영 사항 및 코멘트 확인했습니다 🤭 고생했어요 마루 ~ !!! 🌞
Issue Number
#270
As-Is
To-Be
Check List
Test Screenshot
_.mov
(Optional) Additional Description
카운트다운 구현
❌ before : 게임 시작 → 게임 시작 API 호출 → 폴링 true → 라우팅
✅ after : 게임 시작 → 게임 시작 API 호출 → 폴링 true → 카운트다운 UI → 라우팅
카운트다운 렌더링 → 3초 후 navigate
방장의 동작에 의해 다른 사용자가 영향을 받으려면 응답받는 서버의 데이터가 변경되어야 한다.
따라서, 폴링으로 받는 isGameStart가 true가 될 때 카운트다운을 시작해야 모두가 같은 화면을 볼 수 있다.
그래서 이를 위해 서버 개발자와 소통하여 문제 상황을 공유하였고, 이전에 피드백을 받았던 내용과 함께 총 4초의 여유초를 제한시간보다 더 두기로 결정하였다.
🌸 Storybook 배포 주소
🌸 Storybook 배포 주소