-
Notifications
You must be signed in to change notification settings - Fork 2
[FIX] 관객 어플리케이션 QA 반영 #87
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
Conversation
✅ Deploy Preview for hufscheer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for hufscheer-manager ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ohprettyhak
left a comment
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 <CheerTalkItem {...cheerTalk} />; | ||
| return <CheerTalkItem {...(cheerTalk.at(-1) as GameCheerTalkWithTeamInfo)} />; |
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.
props를 전달하여 false일 때, 그러니까 OnAir 컴포넌트에서 호출하는 경우에 MenuModal을 제거하자는 말씀이시죠? 좋은 것 같습니다! 마우스 커서도 반영하겠습니다ㅎㅎ
| if (!scrollRef.current) return; | ||
|
|
||
| const scrollTop = scrollRef.current.scrollHeight - scrollHeight; | ||
| scrollRef.current.scrollTop = scrollTop; |
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.
scrollTop 변수를 61번 줄에서만 사용하고 있기 때문에 inline으로 작성해도 괜찮을 거 같습니다.
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.
가로로 너무 길어지는 것 같아 따로 변수에 담아 봤는데, 인라인으로 작성해도 상관없을 것 같습니다! 반영하겠습니다👍 white-space 속성도 반영해두겠습니다ㅎㅎ
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.scrollY를 조작하는 방식인데, 이 코드에서는 그런게 보이지 않아 질문드립니다.
scrollTop에 값을 저장하면 해당 값에 해당하는 높이로 스크롤이 이동하는건가요? 그리고 아래 exhaustive-deps 경고를 꺼둔 이유도 알고 싶습니다!
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.
scrollY는 브라우저의 스크롤 높이를 조작하지만, scrollTop은 특정 요소의 스크롤 높이를 조작한다는 것이 차이입니다. 그래서 두 값은 서로 사용하는 경우가 다른 것 같아요!
eslint-disable 라인을 추가한 이유는, useEffect 내에서 scrollHeight라는 외부 상태를 사용하고 있습니다. 만약 scrollHeight를 의존성 배열에 추가하면 다음과 같은 버그가 생깁니다.
- 처음 컴포넌트가 mount 될 때 한 번 변경
- scrollHeight가 변경되었으니 useEffect의 콜백을 실행
- useEffect의 콜백에서는 scrollHeight를 변경
- 다시 useEffect의 콜백 실행
위와 같은 버그가 발생하기 때문에 scrollHeight를 제거하고 eslint 경고를 무시해주었습니다. 그리고 애초에 scroll 높이는 scrollHeight 상태가 변경될 때마다 새롭게 계산해주어야 하는 값은 아니라서 상관없다고 판단했습니다!
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.
고생하셨습니다! 저도 추가로 코멘트 등록 이후 테스트를 더 진행해봤는데 netlify preview에선 CORS에러 때문에 응원톡이 등록되지 않는게 맞나요? 처음에 욕설을 적지 않았는데 공격적인 단어가 포함돼있다고 alert이 뜨길래 확인해봤더니 CORS에러 때문에 POST 요청이 반려됐다는건 확인했는데, 그럼에도 불구하고 통신 에러와 일치하지 않는 문구의 alert이 뜨는건 개선해야할 사항으로 보입니다!
| }, | ||
| { | ||
| onError: () => { | ||
| alert('공격적인 단어가 포함되어 있습니다.'); |
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.
요거 문구는 디자인팀에 이슈 올려서 확정지으면 좋을 것 같아요. 제 생각에는 응원톡 달 때 욕설이 필터돼 등록이 막힌 경우니, 공격적인 단어가 포함돼있다는 문구보다 좀 더 저지당하는 느낌이 들도록 문구를 정해야할 것 같아요 like '훕치치는 차별, 비방 등 유해한 언어를 일체 금지하고 있습니다. 순화된 언어로 이용해주세요' 이런 느낌?
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.
이 부분은 일단 빠르게 작업하느라 임시로 작성해뒀습니다. 나중에 정식 QA 기간 때 피드백 받고 수정하면 좋을 것 같아요!
| const cheerTalks = useMemo( | ||
| () => (cheerTalkList ? cheerTalkList.pages.flatMap(talk => talk) : []), | ||
| [cheerTalkList], | ||
| ); |
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.
useMemo로 최적화한 것 매우 좋네요!
| if (!scrollRef.current) return; | ||
|
|
||
| const scrollTop = scrollRef.current.scrollHeight - scrollHeight; | ||
| scrollRef.current.scrollTop = scrollTop; |
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.scrollY를 조작하는 방식인데, 이 코드에서는 그런게 보이지 않아 질문드립니다.
scrollTop에 값을 저장하면 해당 값에 해당하는 높이로 스크롤이 이동하는건가요? 그리고 아래 exhaustive-deps 경고를 꺼둔 이유도 알고 싶습니다!
| {groupedGameList.map(gameList => ( | ||
| <Fragment key={gameList.startTime}> | ||
| <div className={styles.dateRow}>{gameList.startTime}</div> | ||
| <ul key={gameList.startTime} className={styles.listRoot}> |
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.
제가 ul 위치를 변경할 때 key 위치를 바꾼다는걸 깜빡했네요 대신 해주셔서 감사합니다! Fragment에 key값이 생긴만큼 ul에는 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.
엇 ul에 있었군요ㅎㅎ 저도 이건 못봤네요. 반영하겠습니다!
| {GAMES.map(game => ( | ||
| <AsyncBoundary | ||
| key={game.key} | ||
| errorFallback={() => game.errorFallback()} | ||
| loadingFallback={game.loadingFallback} | ||
| > | ||
| <GameList state={game.key} /> | ||
| </AsyncBoundary> | ||
| ))} | ||
| </section> | ||
| ); | ||
| } | ||
|
|
||
| type Games = { | ||
| key: GameState; | ||
| errorFallback: () => JSX.Element; | ||
| loadingFallback: JSX.Element; | ||
| }; | ||
|
|
||
| const GAMES: Games[] = [ | ||
| { | ||
| key: 'playing', | ||
| errorFallback: GameList.PlayingErrorFallback, | ||
| loadingFallback: <Loader />, | ||
| }, | ||
| { | ||
| key: 'scheduled', | ||
| errorFallback: GameList.ScheduledErrorFallback, | ||
| loadingFallback: <Loader />, | ||
| }, | ||
| { | ||
| key: 'finished', | ||
| errorFallback: GameList.FinishedErrorFallback, | ||
| loadingFallback: <Loader />, | ||
| }, | ||
| ]; |
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.
추상화해 유지보수하기 쉽게 바꾼 점 매우 좋습니다!👍
|
에러 문구를 출력하는 조건은 말씀하신 것처럼 욕설로 인해 전송에 실패했다는 조건이 있어야 해요. 하지만 현재는 문자열로 해당 정보를 내려주고 있는데, 길이가 조금 있는 문자열이라서 비교 성능이 떨어진다고 판단하여 일단은 에러 발생으로 트리거 되도록 해두었습니다. 이건 백엔드 팀원들에게 지금 이야기해서 반영하겠습니다! |
|
이 QA PR이 반영되지 않은 main 최신 코드를 테스트하는건 의미가 없겠죠? 로컬에서 테스트해봤을 땐 앱 최초 사용 시 실시간 응원톡이 기존 응원톡 데이터가 있음에도 불구하고 모두 디폴트 값이 렌더링된다는 점, 응원톡 등록은 성공하는데 새 응원톡이 안보이다가 특정 시간 이후 한꺼번에 업데이트되고 그 이후로는 정상 동작하는 버그(발동 조건을 모르겠습니다) 등을 발견헀는데, netlify preview에서는 발견되지 않는 버그들이네요..? |
|
|
소켓의 문제일 수 있겠군요... 제가 하단 스크롤을 할 수 있었는데 그 부분을 놓쳐서 착각한건진 모르겠지만 그 버그가 발생했을 때 모달을 껐다 키고 다른 경기의 응원톡 모달을 켰을 때도 동일한 새 응원톡 씹힘 버그를 목격했던 것 같은데, 3번의 문제라면 소켓 데이터가 씹혔는지 여부를 확인해야 재호출을 할지말지 결정할탠데 그걸 아는것도 쉽지 않겠네요. 우선은 엣지케이스인만큼 이슈가 있다 인지하고 넘어가도록 합시다! 고생하셨습니다🙇♂️ |



🌍 이슈 번호
✅ 작업 내용
9999라는 매직 넘버를 상수로 관리할 필요가 있을지 고민했는데, 이 부분에 대해서 의견 남겨주시면 감사하겠습니다!useTimeouthook을 이용하여 100ms 이후 스크롤을 내리도록 수정합니다.document API를 사용하고 있어서 build 시 에러가 터집니다.alert를 사용한 상태고, 추후 수정하면 좋을 것 같습니다.📝 참고 자료
♾️ 기타