Skip to content
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

[FE] 첫 지출내역 추가 시, 입금 상태 관리 Banner 띄우기 #756

Merged
merged 8 commits into from
Oct 17, 2024

Conversation

soi-ha
Copy link
Contributor

@soi-ha soi-ha commented Oct 16, 2024

issue

구현 목적

현재 유저가 입금 상태를 관리할 수 있는 기능을 인지할 방법이 존재하지 않습니다.
그렇기 때문에 유저가 지출내역을 생성했다면, 입금 상태를 관리하는 기능을 인지할 수 있도록 Banner를 활용하여 알려줍니다.

구현 내용

구현된 화면

스크린샷 2024-10-16 16 07 41

입금 상태 관리 Banner 띄우기

참여자 입금 상태 Banner를 띄우는 조건은 다음과 같습니다.

  • 계좌번호를 등록 할 것
  • 최소 1개의 지출내역이 존재할 것
  • sessionStorage에 *closeDepositStateBanner-*${*eventToken*} key가 존재하지 않고 그 값이 false일 경우

참여자 입금 상태 Banner가 닫히는 조건은 단 하나입니다.

  • Banner의 X 버튼을 클릭했을 때

닫히는 조건을 더 추가할까 고민했습니다만, 일단은 x 버튼을 클릭해야만 닫히도록 했습니다. 현 상황에서는 Banner는 최대 1개만 띄워지기 때문입니다.

Banner는 총 2개가 존재하지만 해당 입금상태 Banner는 계좌번호가 존재해야 띄워집니다. 계좌번호 Banne는 계좌번호가 입력되면 사라지기 때문에 2개의 Banner가 동시에 띄워질 일은 발생하지 않습니다. 따라서 입금 상태 Banner에 따로 닫히는 조건을 추가하지는 않았습니다. (만약, 사진 첨부 Banner를 띄우게 된다면 추가로 고려하면 될 것 같습니다.)

입금 상태 관리 Banner 구현

쿠키가 Banner를 너무 잘 만들어줘서.. 쿠키가 작성한 코드를 바탕으로 구현했습니다.

// constants/sessionStorageKeys.ts

const SESSION_STORAGE_KEYS = {
  // ...
  closeDepositStateBannerByEventToken: (eventToken: string) => `closeDepositStateBanner-${eventToken}`,
} as const;
  • closeDepositStateBannerByEventToken

    sessionStorage의 key로 closeDepositStateBanner-${eventToken}를 생성했습니다.

//useAdminPage.ts

// ...

// session storage에 입금 상태관리 배너를 지웠는지 관리
  const storageDepositStateValue = SessionStorage.get<boolean>(
    SESSION_STORAGE_KEYS.closeDepositStateBannerByEventToken(eventId),
  );
  const isClosedDepositState = storageDepositStateValue != null && storageDepositStateValue;

  const isExistStepsAndAccount = steps.length && !isEmptyAccount;
  const [isShowDepositStateBanner, setIsShowDepositStateBanner] = useState<boolean>(
    !!isExistStepsAndAccount && !isClosedDepositState,
  );

  useEffect(() => {
    setIsShowDepositStateBanner(!!isExistStepsAndAccount && !isClosedDepositState);
  }, [isShowDepositStateBanner, steps, bankName, accountNumber]);

  const onDeleteDepositState = () => {
    setIsShowDepositStateBanner(false);
    SessionStorage.set<boolean>(SESSION_STORAGE_KEYS.closeDepositStateBannerByEventToken(eventId), true);
  };
  • storageDepositStateValue

    closeDepositStateBanner-${eventToken} key의 값을 sessionStorage에서 가져옵니다.

  • isClosedDepositState

    closeDepositStateBanner-${eventToken}의 값이 true인지 false인지 저장합니다.
    만약, 값이 true라면 입금 상태 배너는 닫힌 것입니다.

  • isExistStepsAndAccount

    steps(지출내역)와 account(계좌번호)가 존재하는지 확인합니다.

  • isShowDepositStateBanner

    isExistStepsAndAccount가 true이고 isClosedDepositState의 값이 false라면 isShowDepositStateBanner 상태 값은 true입니다.
    isShowDepositStateBanner가 true라는 것은 입금상태 배너가 보인다는 의미입니다. 만약, false라면 배너는 보이지 않습니다.

🚀 트러블슈팅

문제 발생 상황

행사 생성 → 지출 내역 생성 → 계좌번호 입력 → 계좌번호 입력 → adminPage로 이동 → DepositStateBanner 존재 X

해당 flow에서는 DepositStateBanner가 띄워지지 않는 문제가 발생했습니다.

문제 해결

isShowDepositStateBanner의 상태를 변경시키는 useEffect의 의존성 값에 bankName과 accountNumber를 넣어주지 않아서 발생한 문제였습니다. 두 값이 의존성에 존재하지 않아 계좌번호를 입력하였음에도 DepositStateBanner의 상태가 변경되지 않은 것이었습니다.

따라서 bankName과 accountNumber를 의존성에 추가하였습니다.

  • 변경 전

     useEffect(() => {
        setIsShowDepositStateBanner(!!isExistStepsAndAccount && !isClosedDepositState);
      }, [isShowDepositStateBanner, steps]);
  • 변경 후

     useEffect(() => {
        setIsShowDepositStateBanner(!!isExistStepsAndAccount && !isClosedDepositState);
      }, [isShowDepositStateBanner, steps, bankName, accountNumber]);
 // AdminPage.tsx
 
 {isShowDepositStateBanner && (
  <Banner
    onClick={navigateEventMemberManage}
    onDelete={onDeleteDepositState}
    title="참여자 입금상태를 관리할 수 있어요"
    description="어떤 참여자가 입금을 완료했는지 관리해 보세요"
    buttonText="관리하기"
  />
)}

isShowDepositStateBanner의 값이 true이면 위 Banner 컴포넌트를 띄웁니다.
관리하기 버튼을 클릭하면 전체 참여자 관리 페이지로 이동합니다. x 버튼을 클릭하면 버튼이 닫힙니다.

논의하고 싶은 부분(선택)

Banner의 description과 title이 적절한지 확인 부탁드립니다!

참고사항

불필요하게 console이 찍히는 것을 확인하여 해당 코드를 삭제했습니다.
EventPageLayout.tsx에서 mount와 unmount에 관하여 console를 출력하고 있었습니다.

@soi-ha soi-ha added 🖥️ FE Frontend ⚙️ feat feature labels Oct 16, 2024
@soi-ha soi-ha added this to the v2.1.1 milestone Oct 16, 2024
@soi-ha soi-ha self-assigned this Oct 16, 2024
Copy link

Copy link

Copy link

Copy link
Contributor

@jinhokim98 jinhokim98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배너로 사용자에게 입금 상태를 관리할 수 있다고 알려주는 것 좋은 것 같아요.
그러나 배너가 너무 많아진다면 사용자에게 안 좋은 경험을 줄 수 있으니 나중에 배너가 추가된다고 하면 그 때 다시 논의해보면 좋을 것 같아요

setIsShowBanner((bankName === '' || accountNumber === '') && !isClosed);
}, [bankName, accountNumber, isShowBanner]);
setIsShowAccountBanner(isEmptyAccount && !isClosedAccount);
}, [bankName, accountNumber, isShowAccountBanner]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가된 상태에 맞춰서 변수명 바꿔주셔서 고마워요~~

Comment on lines 72 to 73
title="참여자 입금상태를 관리할 수 있어요"
description="어떤 참여자가 입금을 완료했는지 관리해 보세요"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전 좋은 것 같아요~

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금도 굉장히 좋은 UX writing이라고 생각해요!
하지만 조오오오오오금이라도 더 나은 방향이 있을지 생각해 봤을땐,
"입금 완료를 기록한다"라는게 약간 어색하게 다가올 수 있다고 생각해요.
입금 완료를 기록하거나, 체크하는게 조금 더 직관적이지 않을까 싶어서

"어떤 참여자가 입금을 완료했는지 기록해 보세요"
"어떤 참여자가 입금을 완료했는지 체크해 보세요"

도 괜찮을 것 같고,
관리라는 단어에 포커스를 주고 싶다면

"입금한 참여자를 체크해 정산현황을 관리해 보세요"
"입금 여부를 체크해 정산현황을 관리해 보세요"

와 같이 써도 괜찮을 것 같네요!

지금도 충분히 좋아서, 이대로 머지해도 좋으니 approve 하겠습니당 :)
소하의 판단에 맡길게요~!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 기록해보세요가 너무 맘에드네요! 이걸로 변경해두겠습니댱

Comment on lines -46 to -50
console.log('mount');
updateMetaTag('og:title', `행동대장이 "${eventSummary.eventName}"에 대한 정산을 요청했어요`);

return () => {
console.log('unmount');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 반영 감사합니다!

Copy link
Contributor

@Todari Todari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

대 소 하
이 구현을 해나가는 과정에서 유저 흐름과 편의성을 해치지 않게 고려하면서 다양한 분기처리를 진행해 준 것이 너무 인상깊었어요!

오늘의 "기획자가 좋아하는 프론트엔드 개발자"상을 드리도록 하겠습니다.

고생 많았어요~

Comment on lines 44 to 63

// session storage에 입금 상태관리 배너를 지웠는지 관리
const storageDepositStateValue = SessionStorage.get<boolean>(
SESSION_STORAGE_KEYS.closeDepositStateBannerByEventToken(eventId),
);
const isClosedDepositState = storageDepositStateValue != null && storageDepositStateValue;

const isExistStepsAndAccount = steps.length && !isEmptyAccount;
const [isShowDepositStateBanner, setIsShowDepositStateBanner] = useState<boolean>(
!!isExistStepsAndAccount && !isClosedDepositState,
);

useEffect(() => {
setIsShowDepositStateBanner(!!isExistStepsAndAccount && !isClosedDepositState);
}, [isShowDepositStateBanner, steps, bankName, accountNumber]);

const onDeleteDepositState = () => {
setIsShowDepositStateBanner(false);
SessionStorage.set<boolean>(SESSION_STORAGE_KEYS.closeDepositStateBannerByEventToken(eventId), true);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀🚀🚀

Comment on lines 72 to 73
title="참여자 입금상태를 관리할 수 있어요"
description="어떤 참여자가 입금을 완료했는지 관리해 보세요"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금도 굉장히 좋은 UX writing이라고 생각해요!
하지만 조오오오오오금이라도 더 나은 방향이 있을지 생각해 봤을땐,
"입금 완료를 기록한다"라는게 약간 어색하게 다가올 수 있다고 생각해요.
입금 완료를 기록하거나, 체크하는게 조금 더 직관적이지 않을까 싶어서

"어떤 참여자가 입금을 완료했는지 기록해 보세요"
"어떤 참여자가 입금을 완료했는지 체크해 보세요"

도 괜찮을 것 같고,
관리라는 단어에 포커스를 주고 싶다면

"입금한 참여자를 체크해 정산현황을 관리해 보세요"
"입금 여부를 체크해 정산현황을 관리해 보세요"

와 같이 써도 괜찮을 것 같네요!

지금도 충분히 좋아서, 이대로 머지해도 좋으니 approve 하겠습니당 :)
소하의 판단에 맡길게요~!

@@ -1,5 +1,6 @@
const SESSION_STORAGE_KEYS = {
closeAccountBannerByEventToken: (eventToken: string) => `closeAccountBanner-${eventToken}`,
closeDepositStateBannerByEventToken: (eventToken: string) => `closeDepositStateBanner-${eventToken}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 의견이 궁금한데, 여기에 WithEventId(토큰이긴한데 '지금부터 토큰으로 개발합시다!!' 하진 않아서 레거시 이름을 갖고있는 타입)로 사용하는건 별로일까요? string타입도 '어떤 형식을 갖는 string'인지를 타입으로 지정할 수 있었던 것 같아서요. 혹시나 이 eventToken타입을 좁힐 때가 온다면 하나의 타입을 재사용하는게 수정을 빨리할 수 있지 않을까 싶은 생각이 들어요. 이렇게 하라는건 아니고 정말 생각이 궁금해욧..!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아~ eventToken을 사용한 것은 기존에 작성된 쿠키의 코드를 바탕으로 개발해서 그대로 사용했어요!
레거시 이름인 eventId를 사용하는게 더 좋을지는 프론트엔드 팀원 모두와 이야기를 나눠봐야 겠네요...
웨디 말대로 eventId를 사용한다면 나중에 eventToken으로 변경했을 때, 한번에 변경이 가능해서 편할 것 같긴 하네요!

const isClosed = storageValue !== null && storageValue === true;
// session storage에 계좌번호 입력 배너를 지웠는지 관리
const storageAccountValue = SessionStorage.get<boolean>(SESSION_STORAGE_KEYS.closeAccountBannerByEventToken(eventId));
const isClosedAccount = storageAccountValue !== null && storageAccountValue === true;
Copy link
Contributor

@pakxe pakxe Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isClosedAccound는 '닫힌 계좌냐?'라는 의미로 보이는데 혹시 이 변수는 정확히 어떤 의미인가요?
아래에 사용되는 모습을 보니 '이전에 계좌 번호 등록 배너를 닫은적이 있냐?'변수인 것 같기도 한데요.. 맞나요..?

개인적으로는 isShowAccountBanner라는 변수와 헷갈리는 느낌이 있습니다. 하지만 의미를 설명해주시면 금방 이해할 수 있을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

계좌 배너가 닫혔는지를 뜻합니다!
웨디가 말해준 것 처럼 이미 isShowAccountBanner가 존재하기 때문에 isClosedAccount만을 사용한 것입니당.
isClosedAccount는 세션 스토리지에 배너가 닫혔는지가 기록됐는지만 확인합니다. 이 변수는 isShowAccountBanner를 위한 조건문으로만 활용되어 이렇게 변수를 짓게 되었어요!
추후에도 혼동이 될 것 같으면 수정하는 것도 좋을 것 같습니다!

const onDelete = () => {
setIsShowBanner(false);
const onDeleteAccount = () => {
setIsShowAccountBanner(false);
SessionStorage.set<boolean>(SESSION_STORAGE_KEYS.closeAccountBannerByEventToken(eventId), true);
};

Copy link
Contributor

@pakxe pakxe Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 useAdminPage라는 훅이 수행하는 기능의 구현 로직이 많아보이는 것 같아요.
특히나 여러 배너에 대한 상태를 이곳에서 모두 관리하고 있기 때문에 더 그런 느낌이 드는데요.

배너 상태를 관리하는 용도인 훅을 분리해도 좋을 것 같아요.
예를 들자면 useBanner라고 만들고 return값은 실제 Banner로 합니다.

그리고 {계좌번호배너떠야하니? && <Banner 계좌번호배너/>}로 사용처에서 사용하는 방법도 있는데요. 제가 생각하기엔 useBanner에서 return하는 Banner를 그대로 <Banner />로 렌더링한다면 사용처에서 각 배너들을 모두 import해서 어떤 배너가 있는지 알아야하는 책임이 없어도 되어 좋을 것 같아요. useBanner에서 현재 어떤 배너가 떠야하는지 책임을 떠안는거죠. 사용처에서는 그냥 Banner만 렌더링해주면 끝이구요.

이런 분리에 대해선 어떻게 생각하는지, 분리해서 생기는 미처 생각하지 못한 사이드이펙트가 있는지 소하의 의견이 궁금해요~! 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞아요! 그래서 Banner 관리를 분리할까? 생각을 했다가 까먹었네요...
근데 useAdminPage가 사실 Banner 상태를 관리하는 일만 해서.. 분리를 해야 할까 싶지만? 사실 useAdminPage에 useTotalExpenseAmountStore Hook도 따로 분리해서 불러오고 있네용.
기능 분리를 위해서 Banner와 관련된 것들은 useBanner로 분리하는게 깔끔할 것 같긴 합니당! 시간되면 이번에 같이 반영해볼게요~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

banner와 관련된 상태들을 useBanner 훅으로 분리해줬습니다!
상세한 내용은 여기를 참고해주세용

Copy link
Contributor

@pakxe pakxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굉장히 좋은 기능이라고 생각해요~!

이제 참여자 입금 상태를 관리할 수 있다는 사실을 더 많은 사용자가 인지할 수 있게 되었네욧!!

Copy link

@soi-ha
Copy link
Contributor Author

soi-ha commented Oct 17, 2024

Banner와 관련된 기능들을 useBanner 훅으로 분리해줬습니다! ee79b8b
useAdminPage에서 banner 기능만 존재하는 것이 아니라 총 지출 금액과 관련된 hook들이 존재했습니다. 그렇기 때문에 banner 관련 기능 또한 Hook으로 분리해주는 것이 역할 분리가 되어 좋을 것이라고 판단했습니다~

useBanner에서 evenId, bankName, accountNumber, steps를 props로 받고 있습니다. 해당 Hook에서 호출해도 되지만 useAdminPage에서 다른 기능을 위해 이미 호출되고 있어, 중복 호출을 줄이고자 위와 같이 진행했습니다.

@Todari Todari merged commit f86c6ab into fe-dev Oct 17, 2024
1 of 2 checks passed
@Todari Todari deleted the feature/#729 branch October 17, 2024 06:26
Copy link

@Todari Todari mentioned this pull request Oct 17, 2024
Todari added a commit that referenced this pull request Oct 17, 2024
* feat: 계좌번호가 존재하고 지출내역이 있을 때, 입금 상태관리 배너 띄우기

* feat: 입금상태 배너 버튼 이동 페이지 변경

* chore: 참여자 입금 상태 배너 description 수정

* fix: 지출내역 추가 -> 계좌번호 입력 flow일때 입금상태 관리 배너가 띄워지지 않는 오류 해결

* chore: 불필요한 console.log삭제

* chore: 입금 상태 Banner의 description 수정

* refactor: banner 관련 상태를 useBanner Hook으로 분리

---------

Co-authored-by: TaehunLee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants