feat: 새로운 메시지 저장 시 refetch 전까지 이전 메시지들을 보여주는 오류를 해결 #498
Conversation
There was a problem hiding this comment.
도리~ 🐠🐠🐠 고생했어요!
그런데 refetch onSuccess에 Form을 닫는 로직을 넣어도 해당 오류가 해결되지 않는 것 같아요. 😂
메시지 작성을 여러 번 테스트해봤을 때 전체적으로 메시지 정렬이 밀렸다가 다시 돌아오는 상황이 있습니다. 화면이 깜빡이는 느낌이 들어요.
아래 캡쳐 영상은 저장 버튼 클릭 후 구간에 속도 조절을 걸어둔 영상입니다.
보면 Form이 사라지기 전에 저장된 메시지 목록이 업데이트 되고, 이후 폼이 사라지는 걸 확인할 수 있어요.
slomotion.mp4
생각해보면 아래 조건들이 영향을 미칠 것 같아요.
- onSuccess 로직이 언제 호출되는가, 새로운 messageList가 반환된 전? 후?
- onSuccess 로직 호출 후 Form 제거 렌더링 과정에 얼마나 딜레이가 있는가
새로운 messageList가 내려간 후 onSuccess 로직이 실행되는 경우
폼과 함께 새로운 메시지 목록이 보인 후 onSuccess의 폼 닫기 로직이 실행된다.
onSuccess 로직이 실행되는 경우
onSuccess의 폼 닫기 로직이 실행되어 기존 메시지 목록이 보인 후, 새로운 메시지 목록이 보인다.
이 상황 때문에 메시지 영역에 렌더링할 엘리먼트 리스트 자체를 (하나의 상태로) 관리하는 게 낫지 않을까 생각했었고요.
타이밍이 잘 맞으면 폼 닫기와 새로운 메시지 목록으로 업데이트가 동시에 일어나는 것 같아요.
한번 고민해보고 의견 주세요~!
| <PageTitleWithBackButton>{rollingpaper.title}</PageTitleWithBackButton> | ||
| <main> | ||
| <LetterPaper | ||
| isWrite={isWrite} | ||
| to={rollingpaper.to} | ||
| recipientType={rollingpaper.recipient} | ||
| messageList={[...rollingpaper.messages]} | ||
| handleWriteButtonClick={handleWriteButtonClick} | ||
| onEditEnd={handleWriteEnd} | ||
| /> | ||
| </main> | ||
| </> |
There was a problem hiding this comment.
LetterPaper 컴포넌트는 사실상 레이아웃을 위한 컨테이너 역할인 거 같아요.
굳이 한단계 추상화하는 것보다 LetterPaper의 로직을 RollingpaperPage로 옮겨도 괜찮을 것 같은데 어떤가요?
There was a problem hiding this comment.
이거를 옮겨볼려고 했는데요 useMemo를 쓰려고 하니 api 호출 시 에러가 발생하는(rollingpaper empty상태) 경우때문에 한 파일에서 작성하기 어려울 것 같더라고요...? 나중에 errro처리를 error boundary에서 다 해주거나하면 옮길 수 있을 것 같기도 하네요
|
isWrite을 rollingpaperState로 변경하고 이가 한가지 걸리는 점은 메시지 삭제하는 동안 수정 연필 모양이 꺼졌다 다시 켜지는건데요... 이를 위해 따로 state를 주자니 뭔가... 굳이 싶기도 해서 우선은 edit과 동일하게 진행했습니다. 네이밍을 전반적으로 바꾸서 컨벤션에 안맞는 경우도 조금 존재할 수도 있을 것 같아요! 보면서 알려주세요 💕 |
There was a problem hiding this comment.
도리~ 고생했어요! 👍👍👍
전체적으로 여기 로직이 복잡하네요. 😥
페이지 - 메시지 리스트 - 메시지 박스 각 수준의 컴포넌트에서 어떤 상태를 가지고 어떤 작업이 이뤄지는 게 맞는지 생각해보면서 전체적인 구조 개선을 같이 고민해보면 좋겠고요. 구조 개선은 여기 error fix 이슈에서 진행하지 말고, 전략을 세운 다음 다른 이슈로 진행하면 좋겠습니다.
아래 코멘트와 질문에 대한 답변 남겨주시고 리뷰 요청 다시 주세요!
- 먼저 useMessageWrite는 Letterpaper로 옮겨도 될 것 같아요. 한번 확인해주세요.
- refetchOnWindowFocus 옵션을 default로 주는 것에 대해 먼저 도리의 의견은 어떤가요?
- 폼을 새로 추가하는 것과 기존에 있던 메시지를 수정하면서 폼으로 바뀌는 건 별개의 상황인 것 같아요. 메시지를 수정하는 상황이더라도 연필 아이콘은 나와있는 게 맞다고 생각합니다.
| } | ||
|
|
||
| const useMessage = ({ id, rollingpaperId }: UseMessageProps) => { | ||
| const useMessage = ({ |
There was a problem hiding this comment.
얘 이름이 useMessage네요. useMessageBox로 변경해야겠습니다.
There was a problem hiding this comment.
놓치고 있었는데 굿굿굿 e835b8d
| const { | ||
| rollingpaperState, | ||
| handleWriteButtonClick, | ||
| handleWriteEnd, | ||
| handleEditButtonClick, | ||
| } = useMessageWrite(); |
There was a problem hiding this comment.
이 친구는 LetterPaper 내로 들어가도 되겠네요. 꼭 RollinpaperPage에 있어야 할 이유가 없는 것 같은데, 한번 확인해주세요!
There was a problem hiding this comment.
handleWriteEnd 때문에 상단에서 다 관리하자! 라는 생각으로 구현한 것 같아요. 이 훅도 그렇고 전반적으로 LetterPaper 컴포넌트를 따로 두는 것보다 index 내부에서 전부 처리해주는 것이 prop drilling도 적게 일어나고 코드를 알아보기 좋을 것 같은데... 어떻게하면 하나의 컴포넌트에 다 데려올 수 있을지 고민해봐야겠어요 🤔
There was a problem hiding this comment.
그럼 일단은 index.tsx에서 useMessageWrite를 사용하는 방식을 유지하는 걸까요?
그대로 index.tsx에 있어서 물어봅니다.
There was a problem hiding this comment.
네네 이거를 옮길려고 하니까 복잡해지는 것 같더라고요. 밑에 코멘트 준 대로 context를 도입해야하나 생각이 들기도 하고 구조를 좀 바꾸면서 위치도 옮겨주는게 좋을 것 같아요!
|
저도 코드를 보며... 너무 복잡하고 요상한데라는 생각이 계속 들더라고요ㅠㅠ 어떻게 컴포넌트를 분리해야 로직이 복잡하지 않고 리펙토링하기 쉬울지는 고민해봐야할 것 같아요 🥲
|
prefer2
force-pushed
the
feature/497-letter-refetch
branch
from
667caf7
to
3872f8e
Compare
시도Letterpaper component를 따로 두지 않고 index에 넣기 위한 시도를 해보았습니다. 이전에 이렇게 옮기고자 할 때의 문제점이 Rollingpaper의 길이가 0일때, 그리고 로딩상태일때 때문이었는데요, 이를 처리하기 위해 추가적으로 메시지가 없을 때 empty state를 보여주는 것이 더 좋을까 라는 생각이 들기도 합니다. 롤링페이퍼의 길이를 고정해두고(회색 배경 부분) 길이가 늘어날 때만 늘리는게 보이게 더 좋을 것 같기도 해요. 이에 대해서는 어떻게 생각하나요?? 다만 Loading이라는 상태를 초기 상태로 가지고 있는 것이 맞는가...는 고민이 됩니다. 네이밍같은 부분은 이 로직이 괜찮다면 수정하도록 하겠습니당 |
There was a problem hiding this comment.
고생하셨습니다. Approve 할게요!
위 코멘트에 대한 답변 한번만 남겨주세요!
refetchOnWindowFocus를 false로 두는거 찬성입니다! 저희 프로젝트에서는 이 옵션의 장점을 못 느낀 것 같아요. 지금 error처리하는 곳에서 defaultOption을 처리해주고 있어서 그 브렌치에서 같이 설정해주는건 어떤가요...?
- 좋습니다~ 그렇게 하시죠! 👍
기존의 내편을 들어가보니 연필 아이콘은 보이면서 클릭이 안되는 걸로 되어있군요! 기존처럼 아이콘은 보이면서 클릭이 안되는 걸로 수정하도록 할께요~
- 잘 수정해주셨네요. 확인 완료! 🤗
추가적으로 메시지가 없을 때 empty state를 보여주는 것이 더 좋을까 라는 생각이 들기도 합니다. 롤링페이퍼의 길이를 고정해두고(회색 배경 부분) 길이가 늘어날 때만 늘리는게 보이게 더 좋을 것 같기도 해요. 이에 대해서는 어떻게 생각하나요??
- 롤링페이퍼 페이지의 최소 높이를 지정해두고, empty state를 보여주는 게 좋다고 생각합니다.
좋은 의견이에요. 👍 다른 이슈로 진행합시다!
다만 Loading이라는 상태를 초기 상태로 가지고 있는 것이 맞는가...는 고민이 됩니다. 네이밍같은 부분은 이 로직이 괜찮다면 수정하도록 하겠습니당
- 오,, LOADING이라는 상태가 추가되었네요..!
저는 useMessageWrite에서 롤링페이퍼의 LOADING, NORMAL, WRITE, EDIT 상태와 각 상태를 업데이트하는 핸들러를 반환하고 이걸 다 내려줘서 사용하는 게 복잡하게 느껴져요. 이런 상태 정의가 맞을까라는 의문도 있고, 또 이렇게 내려줄거면 RollingpaperPage에서의 context를 만들어 사용하는 게 낫겠다는 생각도 듭니다. 우선 로직 개선 방향에 대한 고민과 작업은 이후 이슈에서 이어갑시다..!
|
메시지 수정 시 색상변경을 해본 결과. 어떤 경우에는 반짝임이 발생하고 대부분의 경우에는 반짝임이 발생하지 않더라고요. 우선 문제점 적어놓고 구조를 다시보며 생각해보겠습니다 |
지금의 깜빡임은 메시지 폼이 사라지는 시점이 잘못되어서라고 생각했습니다. 메시지 폼은 refetch가 성공한 시점에 사라져야하는데 mutation이 성공한 시점에 사라져서 생기는 오류였습니다. 메시지 폼 열기 여부를 롤링페이퍼를 가져오는 시점으로 변경하여 해결했습니다
아쉬운점
refetchOnWindowFocus옵션을 false로 주었습니다. default로 false로 주는 것이 좋을지 고민입니다.나름 다양하게 테스트해보았는데 리뷰어님도 한 번 확인해주세요😘
closed #497