-
Notifications
You must be signed in to change notification settings - Fork 2
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] feat : 리뷰 생성 중 UI 구현 #1064
base: develop
Are you sure you want to change the base?
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.
궁금한 점 코멘트로 남겼어요!
그리고 컴포넌트 위치 관련 제안이 있어요.
지금 TextBlinkLoader가 components/common에 바로 들어가있는데, components/load나 loading을 새로 만드는 건 어떤가요?
저도 작성한 리뷰 페이지 작업하다가 상세 리뷰에 단독으로 걸어줄 로딩 UI가 필요했는데, 기본 높이가 있는 부분이라 기존 LoadingPage에서 사용하던 LoadingBar를 가져오려고 했거든요. 그런데 LoadingBar가 페이지 안에 들어가 있어서 공통으로 빼버릴까 싶었던 차였습니다.
suspense와 마찬가지로 fallback UI를 보여주는 error도 별도의 폴더가 있어서 제안해봅니다!
@@ -42,7 +42,7 @@ const URLGeneratorButton = ({ | |||
onClick={handleURLCreationButtonClick} | |||
disabled={!isFormValid && !mutation.isPending} | |||
> | |||
{mutation.isPending ? '리뷰 링크 생성 중...' : '리뷰 링크 생성하기'} | |||
{mutation.isPending ? <TextBlinkLoader $content="리뷰 링크 생성 중..." /> : <p>리뷰 링크 생성하기</p>} |
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.
children으로 pending일 때 보여줄 문구를 받는 게 조금 더 자연스러워 보이는데 content를 받는 이유는 애니메이션 효과를 넣기 위함이었겠죠?!
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.
처음에는 적용범위를 '텍스트'로 한정해서, before의 content에 넣었어요.
올리가 로딩을 구현하고 있다고 들으니 깜박이는 로딩 적용 범위를 텍스트 이외의 Node로 넓혀도 괜찮겠다는 생각이 들었어요.
현재 피그마 상에서는 깜박이는 로딩이 텍스트외에서 사용하는 곳이 없지만, 애니메이션 자체가 텍스트이외에 모든 요소에도 적용되는 상황이여서, children을 받도록 props 변경하고 컴포넌트명도 BlinkLoader로 변경했어요.
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.
굿굿👍 저도 loading 새로 파서 넣어야겠네요.
children을 받으니 이미지가 포함된 로딩 컴포넌트도 쉽게 사용할 수 있을 것 같아요!
animation: l1 1s linear infinite alternate; | ||
|
||
&::before { | ||
content: '${({ $content }) => $content ?? 'Loading...'}'; |
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.
기본값 처리 굿굿!!
그런데 content는 기본적으로 사용자에게 직접 보여주는 값이기도 한데 $를 붙인 이유가 궁금합니다
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.
'content는 기본적으로 사용자에게 직접 보여주는 값이기도 한데 $를 붙였다'라는게 스타일 컴포넌트의 Props로 받은 이유에 대해 궁금한 건가요?
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.
처음 구현 시 깜박이는 애니메이션 대상을 텍스트로 잡았었고, 애니메이션이 주 작업이라서 css 코드에 집중하다가 content 속성을 활용했어요
- 컴포넌트명 변경 : TextBlinkLoader -> BlinkLoader - common 하위에 loading 폴더 생성 및 컴포넌트 폴더 위치 이동
변경 사항현재 피그마 상에서는 깜박이는 로딩이 텍스트외에서 사용하는 곳이 없어요. 그렇지만 앞으로 다른 로딩 요소에도 활용될 가능성을 고려하여, 모든 요소에 적용할 수 있도록 재사용성을 확장했어요. (props로 children 받도록 바꾸기만 하는 간단한 상황이라 부담없이 진행했습니다)
BlinkLoader는 애니메이션만 지정하고 있기 때문에 기본적으로 폰트 사이즈, 색상 등은 부모 요소의 스타일을 따르며 추가 스타일을 children에 설정하면 됩니다. |
loaindg 폴더 만들어서 따로 뺐어요. |
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.
layout shift를 고려해서 깜빡이는 UI를 구현해주었는데, 의미 전달도 되면서 적절하다고 생각해요. 컨벤션 관련해서 코멘트 하나 남겼으니 확인 부탁해요! 고생했어요👍🏻
interface BlinkLoaderProps extends EssentialPropsWithChildren, S.LoaderProps {} | ||
|
||
const BlinkLoader = ({ children, $animationDurationTime }: BlinkLoaderProps) => { | ||
return <S.Loader $animationDurationTime={$animationDurationTime}>{children}</S.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.
컴포넌트에서 받는 Props명은 $
를 안 붙이고, style props명에만 $
를 붙이기로 했던 것 같아요. 확정지었는지 기억이 확실치 않아서 코멘트 남겨요😅
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.
children으로 더 많은 요소들이 올 수 있게 확장되어 다양하게 사용될 수 있을 것 같아요👍🏻
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명은
$
를 안 붙이고, style props명에만$
를 붙이기로 했던 것 같아요. 확정지었는지 기억이 확실치 않아서 코멘트 남겨요😅
그런 논의가 있었는지 저도 잘 기억이 나지 않네요. 다만, 저번에 진행한 회원용 공통 UI 작업에서 스타일 컴포넌트의 props를 확장 받아서 사용하는 코드들이 있었고 스타일 Props의 별다를 props를 받는게 아니라서 스타일 컴포넌트 props를 확장받는 것으로 했어요.
만약 이 부분에 대한 코드 컨벤션 필요가 느껴진다면, 프론트 회의에서 논의해보죠
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.
추가로 코멘트 남길 부분은 없습니다! 즐거운 설 연휴 보내요😄
🚀 어떤 기능을 구현했나요 ?
button.mov
왜 글씨를 깜빡이는 애니메이션을 사용했는가?
구현 시 아래를 염두해 두고 구현했어요. 스피너의 경우 일정한 높이가 있어야 사용자에 눈에 띄는데 이럴 경우 Layoutshift가 발생해요. 이외에도 여러 애니메이션 효과를 적용해 봤는데, 가장 깔끔하고 고려 조건을 중족하는게 글씨를 깜박이는 거였어요.
구현 시 고려한 사항
🔥 어떻게 해결했나요 ?
TextBlinkLoader의 확장성 범위