feat: 새로운 메시지 저장 시 refetch 전까지 이전 메시지들을 보여주는 오류를 해결 #498

prefer2 · 2022-10-01T05:40:47Z

지금의 깜빡임은 메시지 폼이 사라지는 시점이 잘못되어서라고 생각했습니다. 메시지 폼은 refetch가 성공한 시점에 사라져야하는데 mutation이 성공한 시점에 사라져서 생기는 오류였습니다. 메시지 폼 열기 여부를 롤링페이퍼를 가져오는 시점으로 변경하여 해결했습니다

수정을 하다 이제는 initMessage가 불필요하다는 점을 알게 되어 이를 삭제했습니다(훅으로 빼 놓아서 알아서 초기화가 됨)

아쉬운점

onSuccess마다 메시지 폼을 닫게 됩니다. 초기 가져오는 경우에도 닫게 되는데 로직적으로는 맞는 말인 것 같기는 하나 애매하다고 생각할 수도 있을 것 같습니다
이에 따라 useMessageWrite를 index로 옮겨야 했습니다. prop drilling이 조금 늘었습니다
화면 전환시에 리패치가 일어나는 것이 적절하지 않다고 생각되어 이 훅에서는 refetchOnWindowFocus 옵션을 false로 주었습니다. default로 false로 주는 것이 좋을지 고민입니다.
우선 스낵바가 수정하는 곳에서 나타나게 되어 스낵바가 나타나고 -> 이후에 다시 리패치한 정보들이 보여집니다. 스낵바가 보이는 시점이 조금 애매한 것 같기도 합니다. 다만 이를 리패치시로 옮기자니 첫번째로 정보를 가져올때도 스낵바가 나타나는 문제점이 있어… 우선은 기존의 방식을 유지했습니다

나름 다양하게 테스트해보았는데 리뷰어님도 한 번 확인해주세요😘

closed #497

soyi47

도리~ 🐠🐠🐠 고생했어요!
그런데 refetch onSuccess에 Form을 닫는 로직을 넣어도 해당 오류가 해결되지 않는 것 같아요. 😂

메시지 작성을 여러 번 테스트해봤을 때 전체적으로 메시지 정렬이 밀렸다가 다시 돌아오는 상황이 있습니다. 화면이 깜빡이는 느낌이 들어요.

아래 캡쳐 영상은 저장 버튼 클릭 후 구간에 속도 조절을 걸어둔 영상입니다.
보면 Form이 사라지기 전에 저장된 메시지 목록이 업데이트 되고, 이후 폼이 사라지는 걸 확인할 수 있어요.

slomotion.mp4

생각해보면 아래 조건들이 영향을 미칠 것 같아요.

onSuccess 로직이 언제 호출되는가, 새로운 messageList가 반환된 전? 후?
onSuccess 로직 호출 후 Form 제거 렌더링 과정에 얼마나 딜레이가 있는가

새로운 messageList가 내려간 후 onSuccess 로직이 실행되는 경우

폼과 함께 새로운 메시지 목록이 보인 후 onSuccess의 폼 닫기 로직이 실행된다.

onSuccess 로직이 실행되는 경우

onSuccess의 폼 닫기 로직이 실행되어 기존 메시지 목록이 보인 후, 새로운 메시지 목록이 보인다.

이 상황 때문에 메시지 영역에 렌더링할 엘리먼트 리스트 자체를 (하나의 상태로) 관리하는 게 낫지 않을까 생각했었고요.
타이밍이 잘 맞으면 폼 닫기와 새로운 메시지 목록으로 업데이트가 동시에 일어나는 것 같아요.

한번 고민해보고 의견 주세요~!

soyi47 · 2022-10-02T07:00:24Z

frontend/src/pages/RollingpaperPage/index.tsx

      <PageTitleWithBackButton>{rollingpaper.title}</PageTitleWithBackButton>
      <main>
        <LetterPaper
+          isWrite={isWrite}
          to={rollingpaper.to}
          recipientType={rollingpaper.recipient}
          messageList={[...rollingpaper.messages]}
+          handleWriteButtonClick={handleWriteButtonClick}
+          onEditEnd={handleWriteEnd}
        />
      </main>
    </>


LetterPaper 컴포넌트는 사실상 레이아웃을 위한 컨테이너 역할인 거 같아요.
굳이 한단계 추상화하는 것보다 LetterPaper의 로직을 RollingpaperPage로 옮겨도 괜찮을 것 같은데 어떤가요?

이거를 옮겨볼려고 했는데요 useMemo를 쓰려고 하니 api 호출 시 에러가 발생하는(rollingpaper empty상태) 경우때문에 한 파일에서 작성하기 어려울 것 같더라고요...? 나중에 errro처리를 error boundary에서 다 해주거나하면 옮길 수 있을 것 같기도 하네요

prefer2 · 2022-10-03T04:27:00Z

isWrite을 rollingpaperState로 변경하고 이가 normal, write, edit세가지 state를 가질 수 있도록 하였습니다.
전반적으로 refetch를 해올 때 혹은 초기에 이 state가 normal이 됩니다. 새 메시지 작성, 수정, 수정 취소, 삭제 시 동작을 확인해보기는 했는데 혹시 또 오류가 있을수도 있으니 한 번 동작해보면 좋을 것 같아요.

한가지 걸리는 점은 메시지 삭제하는 동안 수정 연필 모양이 꺼졌다 다시 켜지는건데요... 이를 위해 따로 state를 주자니 뭔가... 굳이 싶기도 해서 우선은 edit과 동일하게 진행했습니다.

네이밍을 전반적으로 바꾸서 컨벤션에 안맞는 경우도 조금 존재할 수도 있을 것 같아요! 보면서 알려주세요 💕

soyi47

도리~ 고생했어요! 👍👍👍

전체적으로 여기 로직이 복잡하네요. 😥
페이지 - 메시지 리스트 - 메시지 박스 각 수준의 컴포넌트에서 어떤 상태를 가지고 어떤 작업이 이뤄지는 게 맞는지 생각해보면서 전체적인 구조 개선을 같이 고민해보면 좋겠고요. 구조 개선은 여기 error fix 이슈에서 진행하지 말고, 전략을 세운 다음 다른 이슈로 진행하면 좋겠습니다.

아래 코멘트와 질문에 대한 답변 남겨주시고 리뷰 요청 다시 주세요!

먼저 useMessageWrite는 Letterpaper로 옮겨도 될 것 같아요. 한번 확인해주세요.
refetchOnWindowFocus 옵션을 default로 주는 것에 대해 먼저 도리의 의견은 어떤가요?
폼을 새로 추가하는 것과 기존에 있던 메시지를 수정하면서 폼으로 바뀌는 건 별개의 상황인 것 같아요. 메시지를 수정하는 상황이더라도 연필 아이콘은 나와있는 게 맞다고 생각합니다.

soyi47 · 2022-10-06T06:14:15Z

frontend/src/pages/RollingpaperPage/hooks/useMessageBox.tsx

 }

-const useMessage = ({ id, rollingpaperId }: UseMessageProps) => {
+const useMessage = ({


얘 이름이 useMessage네요. useMessageBox로 변경해야겠습니다.

놓치고 있었는데 굿굿굿 e835b8d

soyi47 · 2022-10-06T06:18:05Z

frontend/src/pages/RollingpaperPage/index.tsx

+  const {
+    rollingpaperState,
+    handleWriteButtonClick,
+    handleWriteEnd,
+    handleEditButtonClick,
+  } = useMessageWrite();


이 친구는 LetterPaper 내로 들어가도 되겠네요. 꼭 RollinpaperPage에 있어야 할 이유가 없는 것 같은데, 한번 확인해주세요!

handleWriteEnd 때문에 상단에서 다 관리하자! 라는 생각으로 구현한 것 같아요. 이 훅도 그렇고 전반적으로 LetterPaper 컴포넌트를 따로 두는 것보다 index 내부에서 전부 처리해주는 것이 prop drilling도 적게 일어나고 코드를 알아보기 좋을 것 같은데... 어떻게하면 하나의 컴포넌트에 다 데려올 수 있을지 고민해봐야겠어요 🤔

그럼 일단은 index.tsx에서 useMessageWrite를 사용하는 방식을 유지하는 걸까요?
그대로 index.tsx에 있어서 물어봅니다.

네네 이거를 옮길려고 하니까 복잡해지는 것 같더라고요. 밑에 코멘트 준 대로 context를 도입해야하나 생각이 들기도 하고 구조를 좀 바꾸면서 위치도 옮겨주는게 좋을 것 같아요!

prefer2 · 2022-10-06T07:55:44Z

저도 코드를 보며... 너무 복잡하고 요상한데라는 생각이 계속 들더라고요ㅠㅠ 어떻게 컴포넌트를 분리해야 로직이 복잡하지 않고 리펙토링하기 쉬울지는 고민해봐야할 것 같아요 🥲

refetchOnWindowFocus를 false로 두는거 찬성입니다! 저희 프로젝트에서는 이 옵션의 장점을 못느낀것 같아요. 지금 error처리하는 곳에서 defaultOption을 처리해주고 있어서 그 브렌치에서 같이 설정해주는건 어떤가요...?
기존의 내편을 들어가보니 연필 아이콘은 보이면서 클릭이 안되는 걸로 되어있군요! 기존처럼 아이콘은 보이면서 클릭이 안되는 걸로 수정하도록 할께요~

prefer2 · 2022-10-06T09:10:26Z

시도

Letterpaper component를 따로 두지 않고 index에 넣기 위한 시도를 해보았습니다. 이전에 이렇게 옮기고자 할 때의 문제점이 Rollingpaper의 길이가 0일때, 그리고 로딩상태일때 때문이었는데요, 이를 처리하기 위해 LOADING이라는 RollinpaperState를 추가해주었습니다. 저만의 상태...라고 생각하고 구현했습니다. LOADING 상태일 때 로더를 보여주고 아닐 때는 롤링페이퍼 상태를 보여줍니다.
빈 롤링페이퍼의 경우 useMemo내부에서 빈 배열을 리턴하는 식으로 처리해놓았습니다.

추가적으로 메시지가 없을 때 empty state를 보여주는 것이 더 좋을까 라는 생각이 들기도 합니다. 롤링페이퍼의 길이를 고정해두고(회색 배경 부분) 길이가 늘어날 때만 늘리는게 보이게 더 좋을 것 같기도 해요. 이에 대해서는 어떻게 생각하나요??

다만 Loading이라는 상태를 초기 상태로 가지고 있는 것이 맞는가...는 고민이 됩니다. 네이밍같은 부분은 이 로직이 괜찮다면 수정하도록 하겠습니당

soyi47

고생하셨습니다. Approve 할게요!

위 코멘트에 대한 답변 한번만 남겨주세요!

refetchOnWindowFocus를 false로 두는거 찬성입니다! 저희 프로젝트에서는 이 옵션의 장점을 못 느낀 것 같아요. 지금 error처리하는 곳에서 defaultOption을 처리해주고 있어서 그 브렌치에서 같이 설정해주는건 어떤가요...?

좋습니다~ 그렇게 하시죠! 👍

기존의 내편을 들어가보니 연필 아이콘은 보이면서 클릭이 안되는 걸로 되어있군요! 기존처럼 아이콘은 보이면서 클릭이 안되는 걸로 수정하도록 할께요~

잘 수정해주셨네요. 확인 완료! 🤗

추가적으로 메시지가 없을 때 empty state를 보여주는 것이 더 좋을까 라는 생각이 들기도 합니다. 롤링페이퍼의 길이를 고정해두고(회색 배경 부분) 길이가 늘어날 때만 늘리는게 보이게 더 좋을 것 같기도 해요. 이에 대해서는 어떻게 생각하나요??

롤링페이퍼 페이지의 최소 높이를 지정해두고, empty state를 보여주는 게 좋다고 생각합니다.
좋은 의견이에요. 👍 다른 이슈로 진행합시다!

다만 Loading이라는 상태를 초기 상태로 가지고 있는 것이 맞는가...는 고민이 됩니다. 네이밍같은 부분은 이 로직이 괜찮다면 수정하도록 하겠습니당

오,, LOADING이라는 상태가 추가되었네요..!
저는 useMessageWrite에서 롤링페이퍼의 LOADING, NORMAL, WRITE, EDIT 상태와 각 상태를 업데이트하는 핸들러를 반환하고 이걸 다 내려줘서 사용하는 게 복잡하게 느껴져요. 이런 상태 정의가 맞을까라는 의문도 있고, 또 이렇게 내려줄거면 RollingpaperPage에서의 context를 만들어 사용하는 게 낫겠다는 생각도 듭니다. 우선 로직 개선 방향에 대한 고민과 작업은 이후 이슈에서 이어갑시다..!

prefer2 · 2022-10-08T06:08:03Z

메시지 수정 시 색상변경을 해본 결과. 어떤 경우에는 반짝임이 발생하고 대부분의 경우에는 반짝임이 발생하지 않더라고요. 우선 문제점 적어놓고 구조를 다시보며 생각해보겠습니다

prefer2 added 2 commits October 1, 2022 14:33

feat: 메시지 작성시 발생하는 깜빡임 제거

569bd30

refactor: 불필요한 initMessage 제거

9866d7b

prefer2 added 프론트엔드 리팩토링 labels Oct 1, 2022

prefer2 requested a review from soyi47 October 1, 2022 05:40

soyi47 requested changes Oct 2, 2022

View reviewed changes

prefer2 added 2 commits October 3, 2022 13:19

feat: rollingpaper state 작성

b8bc69d

feat: rollingpaper state 분류 및 깜빡임 제거

9a693fd

soyi47 requested changes Oct 6, 2022

View reviewed changes

refactor: hook 이름 변경 useMessage -> useMessageBox

e835b8d

prefer2 added 2 commits October 6, 2022 16:58

refactor: 메시지 수정, 삭제시에 메시지 작성 아이콘 보이도록 수정

c0c8250

refactor: recipient 상수화

3872f8e

prefer2 force-pushed the feature/497-letter-refetch branch from 667caf7 to 3872f8e Compare October 6, 2022 08:13

refactor: rollingpaper 상태 정리 및 파일 이동

39808d6

soyi47 approved these changes Oct 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 새로운 메시지 저장 시 refetch 전까지 이전 메시지들을 보여주는 오류를 해결 #498

feat: 새로운 메시지 저장 시 refetch 전까지 이전 메시지들을 보여주는 오류를 해결 #498

prefer2 commented Oct 1, 2022

soyi47 left a comment •

edited

soyi47 Oct 2, 2022

prefer2 Oct 2, 2022

prefer2 commented Oct 3, 2022

soyi47 left a comment

soyi47 Oct 6, 2022

prefer2 Oct 6, 2022

soyi47 Oct 6, 2022

prefer2 Oct 6, 2022

soyi47 Oct 6, 2022

prefer2 Oct 7, 2022

prefer2 commented Oct 6, 2022 •

edited

prefer2 commented Oct 6, 2022

soyi47 left a comment •

edited

prefer2 commented Oct 8, 2022

feat: 새로운 메시지 저장 시 refetch 전까지 이전 메시지들을 보여주는 오류를 해결 #498

Are you sure you want to change the base?

feat: 새로운 메시지 저장 시 refetch 전까지 이전 메시지들을 보여주는 오류를 해결 #498

Conversation

prefer2 commented Oct 1, 2022

아쉬운점

soyi47 left a comment • edited

Choose a reason for hiding this comment

새로운 messageList가 내려간 후 onSuccess 로직이 실행되는 경우

onSuccess 로직이 실행되는 경우

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prefer2 commented Oct 3, 2022

soyi47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prefer2 commented Oct 6, 2022 • edited

prefer2 commented Oct 6, 2022

시도

soyi47 left a comment • edited

Choose a reason for hiding this comment

prefer2 commented Oct 8, 2022

soyi47 left a comment •

edited

prefer2 commented Oct 6, 2022 •

edited

soyi47 left a comment •

edited