-
Notifications
You must be signed in to change notification settings - Fork 5
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] 참여자 별 내역에서 토스로 송금 유도하는 기능 (마무리) #577
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.
엄청 많은 볼륨이었는데 고생 많았어요 쿠키~!~!
증말증말 든든쓰맨
CopyToClipboard 관련해선 다함께 얘기 나눠봐요~!
argTypes: { | ||
isFinish: { | ||
description: '', | ||
control: {type: 'boolean'}, | ||
}, | ||
}, |
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.
꼼꼼한 storybook 작성 매우 좋습니다~! 🚀🚀
}; | ||
|
||
const BankSendButton = ({clipboardText, onBankButtonClick, ...buttonProps}: BankSendButtonProps) => { | ||
const BankSendButton = ({clipboardText, onBankButtonClick, isFinish = false, ...buttonProps}: BankSendButtonProps) => { |
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.
isFinish 보다는 isFinished라고 사용하거나
혹은 우리가 용어 정리에서 정했던 isDeposited
를 사용해도 좋을 것 같네요~
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.
isDeposited를 사용하게 되면 isDeposited (입금 상태)도 같이 완료로 바꿔주어야 할 것 같아요.
이 기능만 들어가면 isDeposited도 찬성입니다.
<Flex justifyContent="center" alignItems="center" gap="0.125rem"> | ||
<Text size="tiny" textColor="black"> | ||
송금완료 | ||
</Text> | ||
</Flex> |
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.
Flex에 children이 하나밖에 없는데 gap 0.125rem 이 있는 이유가 있나요?
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,7 +22,7 @@ function ExpenseItem({name, price, onBankButtonClick, clipboardText, ...divProps | |||
<Flex alignItems="center" gap="0.5rem"> | |||
<Text>{price.toLocaleString('ko-kr')}원</Text> | |||
{isMobileDevice() ? ( | |||
<BankSendButton clipboardText={clipboardText} onBankButtonClick={onBankButtonClick} /> | |||
<BankSendButton clipboardText={clipboardText} onBankButtonClick={onBankButtonClick} isFinish={price <= 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.
price가 음수인 상황도 있을까요?!
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.
음수인 상황은 없지만 혹시나 모르는 상황을 대비해서 (아까와 같은 차등 정산을 조작할 때 마이너스가 나온 버그 등) 0보다 같거나 작은 상황은 정산이 끝났다고 대처를 해둔 것입니다.
css([ | ||
{ | ||
display: 'flex', | ||
justifyContent: 'space-between', | ||
gap: '1rem', | ||
padding: '0.75rem 1rem', | ||
borderRadius: '1rem', | ||
backgroundColor: getBackgroundColorStyle(theme, inputType), | ||
|
||
boxSizing: 'border-box', | ||
boxShadow: getBorderStyle(isFocus, theme, isError), | ||
}, | ||
isAlwaysOnBorder ? inputBoxAlwaysBorderStyle(theme) : inputBoxAnimationStyle(), | ||
]); |
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.
쿠키가 PR 본문에서 말해줬던, isAlwaysOnBorder
에 대해서 생각해 봤는데요,
input의 border는 사실 focus가 되어있다는 것을 보여주기 위함이에요
우리가 지금 UX 개선을 하고 난 뒤에 input 여러개가 한페이지에 있는 케이스가 없어지면서,
autoFocus로 인해서 border가 있는 상황을 주로 보게 되는데, 여전히 input에서 Focus 가 빠지게 되면 border 는 없어지고 있어요.
전체적인 프로젝트에서 focus가 있는 곳 에만 primary color 라인을 사용하고 있는데,
이대로라면 유저는 이곳에선 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.
흐음... 그럴 수도 있겠네요.. 피그마에 이렇게 구현이 되어있어서 이 페이지에는 이렇게 해야 하는구나! 하고 추가했던 건데 통일성을 깨뜨렸다는 생각도 드네요;;
다같이 이야기해봐도 좋을 것 같아요
{elements.map((text, index) => { | ||
return ( | ||
<Text size="subTitle" textColor={emphasize.includes(text) ? 'black' : 'gray'} style={{whiteSpace: 'pre'}}> | ||
<Text | ||
key={`${text}-${index}`} | ||
size="subTitle" | ||
textColor={emphasize.includes(text) ? 'black' : 'gray'} | ||
style={{whiteSpace: 'pre'}} | ||
> |
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.
이곳에서 elements는 정적으로 사용되다 보니, 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.
이게 안 들어가면 window console 창에서 빨간색 경고가 발생해요. 그래서 추가해뒀습니다!
if (bankName === '' || accountNumber === '') { | ||
showToast({ | ||
showingTime: 3000, | ||
message: '잠깐! 정산을 초대하기 전에\n계좌를 등록해주세요.', | ||
type: 'error', | ||
position: 'bottom', | ||
bottom: '8rem', | ||
}); |
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 useRequestPatchEventOutline = () => { | ||
const eventId = getEventIdByUrl(); | ||
|
||
const queryClient = useQueryClient(); | ||
|
||
const {mutateAsync} = useMutation({ | ||
mutationFn: (eventOutline: Partial<Event>) => requestPatchEvent({eventId, eventOutline}), | ||
onSuccess: () => { | ||
queryClient.invalidateQueries({ | ||
queryKey: [QUERY_KEYS.event], | ||
}); | ||
}, | ||
}); | ||
|
||
return { | ||
patchEventOutline: mutateAsync, | ||
}; | ||
}; |
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.
물론 아직 사용되진 않겠지만, 다른 hook들에서도 return에 rest를 반환해 주고 있긴 해요~!
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.
저는 나중에 필요할 때 추가해서 선언해주려고 했었는데 다른 곳에서 그렇게 하고 있다면 통일성을 지켜볼게요
// PATCH /api/admin/events/:eventId (requestPatchEvent) | ||
http.patch<any, {eventName?: string; bankName?: string; accountNumber?: string}>( | ||
`${MOCK_API_PREFIX}${ADMIN_API_PREFIX}/:eventId`, |
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.
MSW 수정까지... 대쿠키
issue
구현 사항
퍼널 디자인에 맞는 계좌번호 입력 화면 구현
계좌 번호 입력 페이지를 임시로 만들어두었다면 이제는 퍼널 디자인이 fe-dev에 반영이 되어 퍼널 디자인을 적용시켰습니다.
추가로 Label input의 옵션을 추가했습니다. 모두 default 값은 false입니다.
은행과 계좌번호
서버로부터 Event 정보를 불러와서 은행과 계좌번호를 가져옵니다.
값이 비었을 경우 (주최자가 등록하지 않은 경우)는 빈 값입니다.
값이 있을 경우 이전에 등록한 값으로 바로 보여집니다.
확인 버튼을 누르면 수정 요청을 기다린 뒤에 admin 페이지로 라우팅을 변경합니다.
계좌번호가 등록되지 않았을 때 공유를 누르면 계좌번호 입력 유도
계좌번호가 등록되지 않은 상태로 링크 공유를 했을 때, 사용자가 토스를 이용해서 송금을 할 수 있는 기능을 이용할 수 없기 때문에 추가했습니다.
하지만 아직 카카오톡 공유에는 넣지 못한 것이 #545 이 머지가 되지 않아 반영할 수 없었어요.
그리고 이 부분을 구현하면서
react-copy-to-clipboard
라이브러리에 대한 고민점이 있습니다. 이는 아래 논의하고 싶은 부분에 적을게요송금액이 0원일 때의 대체 버튼 추가
송금액이 0원이면 보낼 돈이 없으니 토스 송금 기능을 막아야 한다는 의견이 있어서 추가했습니다.
BankSendButton 옵션으로 isFinish를 추가했으며 default는 false입니다.
이 옵션을 true로 설정하게 되면 송금완료 disabled 버튼이 나오게 됩니다.
계좌번호 복사 후 토스 송금 유도
이전에는 계좌번호 api 논의가 안 되어 구현할 수 없어 텍스트로만 대체해두었는데, 논의가 완료되고 api가 구현되어 실제 사용자가 선택한 값이 복사 되도록 했습니다.
형식 : 국민은행 000000-11-222222 10000원
논의하고 싶은 부분
react-copy-to-clipboard의 선언적 형태에 대해서
CopyToClipboard으로 감싸고 그 안에 버튼을 넣는 선언적 방식입니다.
구현하기 간단하지만 이로 인해 발생하는 문제가 있어요
바로 버튼을 눌렀을 때 항상 복사가 일어난다는 점입니다.
계좌번호가 입력되지 않은 경우 입력으로 유도하는 과정에서 텍스트가 항상 복사가 되어버립니다. 그래서
링크가 복사되었어요
토스트가 나오면서 계좌번호 입력으로 이동합니다. (뭔가 부자연스럽다고 생각했어요)여기서 기술검토 내용을 가져와보자면
navigator.clipboard.writeText
메서드는 보안상의 이유로 특정 브라우저 및 버전에서 호환성 문제가 있을 수 있습니다. 특히, HTTPS 환경에서만 작동하고 사용자 제스처 내에서 호출되어야 하며 특정 모바일 환경에서도 정상적으로 작동하지 않습니다.react-copy-to-clipboard
라이브러리는 이러한 제한 사항을 해결하여 다양한 브라우저 환경에서 안정적으로 작동합니다.https는 배포 환경에서만 잘 돌아가면 되고 onClick을 사용해서만 구현하면 제스처 내에서 호출 문제는 없으나
특정 모바일 환경에서도 정상적으로 작동하지 않습니다.
가 문제입니다.라이브러리가 선언적 형태 이외에 다른 방식도 지원했으면 완벽했겠지만 그렇지 않아서;;; 어떻게 위 사진과 같은 문제를 보완할 수 있을지 논의해봐요
🫡 참고사항