-
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
[FEAT] 대기방 UI 수정 및 런칭 데이 피드백 반영 #288
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.
고생하셨습니다 포메~! 사소한 질문과 제안사항 몇가지 남겨봤습니다 확인부탁드려요:)
개인적으로는 UI에 라운드랑 타이머가 저렇게 크게 위치하는 것보단 하단에 작게 작성된 UI를 선호하는 것 같아요 ㅎㅎ 또 타이머도 보여줄거면 초단위로 바꿔야한다고 생각합니돠 UI는 디자인적인 부붕니라 고민을 같이 해봐야겠네용
그리고 useKeyboardUp을 모바일에서 확인하기가 어려워서 화면녹화를 올려주시면 감사하겠숩니다!
@@ -5,6 +5,7 @@ | |||
"main": "index.js", | |||
"scripts": { | |||
"start": "webpack serve --config webpack.config.dev.js", | |||
"start:open": "webpack serve --config webpack.config.dev.js --host 0.0.0.0", |
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.
💭 질문 💭
저는 host 설정 안해도 잘 들어가지던데 윈도우는 설정이 다른가요?? (단순 궁금)
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 user = userEvent.setup(); | ||
customRender(<CategoryContainer />, { initializeState }); | ||
const optionButton = await screen.findByRole('button', { name: '카테고리 설정 버튼' }); |
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.
🙏 제안 🙏
변수명을 settingButton으로 하는 건 어떨까요?? 게임 화면에서 선택지를 옵션이라고 표현하고 있어서 혼동이 있을 것 같습니당
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.
💭 질문 💭
findByLabelText와 비교했을 때 어떤 게 자주 쓰이는지 궁금합니다!
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.
생각해보니 option이라는 이름을 이미 사용하고 있었네요!
수정했습니다
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.
어떤 게 자주 사용되는지는 잘 모르겠는데 findByRole이 기존 테스트에서 사용되어서 일관성을 맞추는 게 좋을 것 같아 생각해 사용했습니다
// Then | ||
await waitFor(() => { | ||
// getByText을 사용하면 해당 텍스트를 찾지 못하면 에러가 발생해서 | ||
// queryByText 사용 |
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.
제거 완료했습니다!
</section> | ||
<> | ||
<button | ||
aria-label="카테고리 설정 버튼" |
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.
🙏 제안 🙏
button 태그에는 aria-label에 버튼이라는 말을 빼는 게 좋을 것 같아요! 스크린 리더로 읽으면 aria-label + 버튼 이라고 읽더라구요
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.
리더기에서 버튼
이라는 이름이 겹칠거라고는 생각 못했네요! 수정했습니다
<button | ||
aria-label="카테고리 설정 버튼" | ||
css={categoryContainerLayout} | ||
onClick={isMaster ? show : () => {}} |
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.
🙏 제안 🙏
onClick 이벤트가 아예 발생하지 않도록 disabled 하는 방향은 잘 안됐었나요?? 연해지는 스타일이 문제라면 disabled 스타일을 수정해보는 것도 좋은 것 같아요! 스타일 수정이 안된다면 애매하기 하겠네욥
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.
방장이 아니면 show 함수 자체가 전달이 안 되서 disabled를 굳이 사용하지 않아도 된다고 판단했습니다!
스타일을 수정할 수 있긴 한데 이미 다른 곳에서는 disabled를 스타일 수정 없이 사용하고 있어서 일관성이 떨어진다고도 생각했어요
frontend/src/hooks/useKeyboardUp.ts
Outdated
}; | ||
window.addEventListener('resize', handleResize); | ||
return () => { | ||
window.addEventListener('remove', handleResize); |
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.
💭 질문 💭
resize 이벤트를 감지하고, remove 이벤트를 또 감지하는 건 어떤 로직인가요??
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.
💭 질문 💭
발생한 이벤트를 removeEventListener를 하지 않아도 괜찮나요?
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.
window.addEventListener('remove', handleResize)
는 제가 잘못 적어서 window.removeEventListener('resize', handleResize)
로 수정했어요!
모바일 브라우저에서 키보드가 위로 올라가면 resize
이벤트가 발생합니다!
frontend/src/hooks/useKeyboardUp.ts
Outdated
@@ -0,0 +1,23 @@ | |||
import { useEffect, useState } from 'react'; | |||
|
|||
export const useKeyboardUp = () => { |
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개니까 export default로 내보내는 게 어떨까용
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.
수정했습니다!
}, [roomUuid, setRoomUuidState]); | ||
|
||
if (nicknameInputRef.current) { | ||
nicknameInputRef.current.focus(); |
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.
첫 페이지 시 닉네임이 focus가 되도록 하는 동작인데 생각해 보니 그냥 HTML의 기본 기능을 쓰면 될 것 같아서 수정했습니다!
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.
저 autofocus로 수정했는데 저희 eslint 컨벤션에서 autofocus는 사용성과 접근성이 떨어져서
금지하고 있네요.... 다시 되돌렸습니다
frontend/webpack.config.dev.js
Outdated
@@ -6,6 +6,7 @@ module.exports = merge(common, { | |||
mode: 'development', | |||
devtool: 'eval-source-map', | |||
devServer: { | |||
open: true, // 개발 서버 시작 시 자동으로 브라우저 열기 |
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.
주석 제거했습니다!
@@ -10,12 +10,12 @@ interface NicknameInputProps { | |||
nicknameInputRef: RefObject<HTMLInputElement>; | |||
handleMakeOrEnterRoom: () => void; | |||
} | |||
const randomNickname = createRandomNickname(); |
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.
닉네임이 컴포넌트가 re-rendering이 될 때 마다 랜덤 닉네임이 바뀌더라고요. 그래서 안 바뀌도록 컴포넌트 밖에서 선언해주었습니다!
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.
피드백 반영했습니다! 그 useKeyboard는 휴대폰에서만 동작하는 기능이라서 화면 녹화가 힘들어 못했네요 ㅠ
이 기능은 직접 보여주는게 좋을 것 같아요
@@ -5,6 +5,7 @@ | |||
"main": "index.js", | |||
"scripts": { | |||
"start": "webpack serve --config webpack.config.dev.js", | |||
"start:open": "webpack serve --config webpack.config.dev.js --host 0.0.0.0", |
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 user = userEvent.setup(); | ||
customRender(<CategoryContainer />, { initializeState }); | ||
const optionButton = await screen.findByRole('button', { name: '카테고리 설정 버튼' }); |
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.
생각해보니 option이라는 이름을 이미 사용하고 있었네요!
수정했습니다
}; | ||
const user = userEvent.setup(); | ||
customRender(<CategoryContainer />, { initializeState }); | ||
const optionButton = await screen.findByRole('button', { name: '카테고리 설정 버튼' }); |
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.
어떤 게 자주 사용되는지는 잘 모르겠는데 findByRole이 기존 테스트에서 사용되어서 일관성을 맞추는 게 좋을 것 같아 생각해 사용했습니다
// Then | ||
await waitFor(() => { | ||
// getByText을 사용하면 해당 텍스트를 찾지 못하면 에러가 발생해서 | ||
// queryByText 사용 |
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.
제거 완료했습니다!
</section> | ||
<> | ||
<button | ||
aria-label="카테고리 설정 버튼" |
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.
리더기에서 버튼
이라는 이름이 겹칠거라고는 생각 못했네요! 수정했습니다
frontend/src/hooks/useKeyboardUp.ts
Outdated
@@ -0,0 +1,23 @@ | |||
import { useEffect, useState } from 'react'; | |||
|
|||
export const useKeyboardUp = () => { |
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.
수정했습니다!
frontend/src/hooks/useKeyboardUp.ts
Outdated
}; | ||
window.addEventListener('resize', handleResize); | ||
return () => { | ||
window.addEventListener('remove', handleResize); |
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.
window.addEventListener('remove', handleResize)
는 제가 잘못 적어서 window.removeEventListener('resize', handleResize)
로 수정했어요!
모바일 브라우저에서 키보드가 위로 올라가면 resize
이벤트가 발생합니다!
@@ -10,12 +10,12 @@ interface NicknameInputProps { | |||
nicknameInputRef: RefObject<HTMLInputElement>; | |||
handleMakeOrEnterRoom: () => void; | |||
} | |||
const randomNickname = createRandomNickname(); |
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.
닉네임이 컴포넌트가 re-rendering이 될 때 마다 랜덤 닉네임이 바뀌더라고요. 그래서 안 바뀌도록 컴포넌트 밖에서 선언해주었습니다!
}, [roomUuid, setRoomUuidState]); | ||
|
||
if (nicknameInputRef.current) { | ||
nicknameInputRef.current.focus(); |
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.
첫 페이지 시 닉네임이 focus가 되도록 하는 동작인데 생각해 보니 그냥 HTML의 기본 기능을 쓰면 될 것 같아서 수정했습니다!
}, [roomUuid, setRoomUuidState]); | ||
|
||
if (nicknameInputRef.current) { | ||
nicknameInputRef.current.focus(); |
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.
저 autofocus로 수정했는데 저희 eslint 컨벤션에서 autofocus는 사용성과 접근성이 떨어져서
금지하고 있네요.... 다시 되돌렸습니다
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.
포메 ~~ 여러 피드백을 반영하시느라 고생많았어요 !!! 몇 가지 코멘트 남겼으니 확인해 주시고 답변주세요 !!!!! 그리고 대기방에 카테고리를 보여주는 디자인을 함께 고민해 보면 좋을 것 같아요 !!!!! 일단 어프루브 드립니다 🌞✨
</section> | ||
<> | ||
<button | ||
aria-label="카테고리 설정 버튼" |
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.
🙏 제안 🙏
혹시 방장이 아니면 설정 버튼이 화면에 안 보이게 만드는 것은 어떻게 생각하세요 ? !
frontend/src/hooks/useKeyboardUp.ts
Outdated
}; | ||
window.addEventListener('resize', handleResize); | ||
return () => { | ||
window.addEventListener('remove', handleResize); |
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.
💭 질문 💭
발생한 이벤트를 removeEventListener를 하지 않아도 괜찮나요?
text={isLoading ? '접속 중...' : '확인'} | ||
bottom={!isKeyboardUp} | ||
radius={isKeyboardUp ? 'small' : undefined} | ||
size={isKeyboardUp ? 'small' : undefined} |
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.
💭 질문 💭
키보드가 올라온 경우 radius가 왜 생기는 건지 궁금해요 ! UI를 확인할 수 없어서 동영상을 PR에 첨부해 주시면 더 좋을 것 같아요 !!!
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.
radius가 없으면 각이 있는 버튼으로 나와서 이를 위해 추가했습니다!
@@ -22,5 +28,10 @@ export const title = css` | |||
`; | |||
|
|||
export const subtitle = css` | |||
font-weight: 800; | |||
font-size: 2rem; |
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.
💭 질문 💭
bold가 아닌 800을 사용한 이유가 있는지 궁금해요 !
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.
더 커스텀하기 쉬울 것 같아서 weight를 사용했습니다!
다른 곳에서도 font를 바꿀 때 주로 weight와 size를 변경해 줬더라고요.
// getByText을 사용하면 해당 텍스트를 찾지 못하면 에러가 발생해서 | ||
// queryByText 사용 | ||
|
||
const roomSetting = screen.queryByText('방 설정'); |
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.
💭 질문 💭
저도 같은 문제를 겪어서 다른 방식으로 해결했는데 queryByText로 하면 찾아지던가요 ?? 저는 이것도 안 됐어서 🥹🥹 혹시 한 스크린에 '방 설정'이 있고 '방'이 있는 경우 여러 엘리멘트가 잡힌다는 에러는 뜨지 않는지도 궁금하네요 .. 이건 차차 답변해 주셔도 괜찮습니다 !!!!
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.
queryByText를 쓰면 해당 텍스트를 못 찾았을 때도 에러가 발생 안 하더라고요! 그래서 modal이 없음을 확인할 때 사용하였습니다.
방 설정
이라는 단어가 포함 되어야지 찾는 것 같더라고요. 여러 엘리먼트가 잡힌 적은 없습니다
<h1 css={title}>{roomSetting.category.label}</h1> | ||
</div> | ||
<div css={roomSettingBox}> | ||
<span css={roomSettingLabel}>타이머</span> |
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.
이미 설정 모달에서 타이머
라는 단어를 사용하고 있어서 맞추는 게 좋을 것 같아 바꾸지 않고 사용했어요.
그리고 다른 옵션에서 이미 라운드
, 카테고리
같은 영단어를 사용 중이라 비슷한 느낌으로 타이머
가 좋을 것 같은 생각입니다!
</div> | ||
<div css={roomSettingBox}> | ||
<span css={roomSettingLabel}>타이머</span> | ||
<h1 css={subtitle}>{roomSetting.timeLimit}</h1> |
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.
🙏 제안 🙏
초 단위로 표시되면 보기 편할 것 같아요! RoomSettingContainer 참고하시면 좋을 듯 합니다!!!!
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.
초 단위로 변경했습니다!
@@ -46,15 +49,11 @@ export const memberList = css` | |||
`; | |||
|
|||
export const inviteButton = css` |
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.
🙏 제안 🙏
화면에서 확인해 보았을 때 연하다는 느낌이 있어, 초대 버튼의 글자를 bold로 처리해주면 더 잘 보일 것 같아요!
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.
글씨 크기에 weight를 주어 더 굵게 만들었습니다!
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.
썬데이 피드백 답변 남겼습니다!
@@ -22,5 +28,10 @@ export const title = css` | |||
`; | |||
|
|||
export const subtitle = css` | |||
font-weight: 800; | |||
font-size: 2rem; |
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.
더 커스텀하기 쉬울 것 같아서 weight를 사용했습니다!
다른 곳에서도 font를 바꿀 때 주로 weight와 size를 변경해 줬더라고요.
// getByText을 사용하면 해당 텍스트를 찾지 못하면 에러가 발생해서 | ||
// queryByText 사용 | ||
|
||
const roomSetting = screen.queryByText('방 설정'); |
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.
queryByText를 쓰면 해당 텍스트를 못 찾았을 때도 에러가 발생 안 하더라고요! 그래서 modal이 없음을 확인할 때 사용하였습니다.
방 설정
이라는 단어가 포함 되어야지 찾는 것 같더라고요. 여러 엘리먼트가 잡힌 적은 없습니다
text={isLoading ? '접속 중...' : '확인'} | ||
bottom={!isKeyboardUp} | ||
radius={isKeyboardUp ? 'small' : undefined} | ||
size={isKeyboardUp ? 'small' : undefined} |
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.
radius가 없으면 각이 있는 버튼으로 나와서 이를 위해 추가했습니다!
<h1 css={title}>{roomSetting.category.label}</h1> | ||
</div> | ||
<div css={roomSettingBox}> | ||
<span css={roomSettingLabel}>타이머</span> |
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.
이미 설정 모달에서 타이머
라는 단어를 사용하고 있어서 맞추는 게 좋을 것 같아 바꾸지 않고 사용했어요.
그리고 다른 옵션에서 이미 라운드
, 카테고리
같은 영단어를 사용 중이라 비슷한 느낌으로 타이머
가 좋을 것 같은 생각입니다!
@@ -46,15 +49,11 @@ export const memberList = css` | |||
`; | |||
|
|||
export const inviteButton = css` |
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.
글씨 크기에 weight를 주어 더 굵게 만들었습니다!
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
#274
As-Is
To-Be
Check List
Test Screenshot
피드백 반영 후 대기방 UI
(Optional) Additional Description
🌸 Storybook 배포 주소