-
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
[Feat/Fix] 모달 레이아웃 & 뷰 퍼블리싱 #316
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.
고생하셨어요 !! 다음부터는 작업량 최대한 분리해서 pr 올리기 ....
리뷰 확인해주세용 ~
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.
기존 common
모달은 그대로 두고, 채원님이 지금처럼 합성 컴포넌트로 구현한 모달을 shared
에 놓고 활용하는게 맞는 것 같아요 !
특정 기능에 따른 모달 컨텐츠를 제공하고, 혹은 각 단계의 step
을 표현해주기도 하니, common
에 있을 친구는 아닌 것 같습니다 !
case 'create-workspace': | ||
return { | ||
icon: | ||
step === totalSteps ? ( | ||
<SuccessIcon width={40} height={40} /> | ||
) : step && totalSteps ? ( | ||
`${step}/${totalSteps}` | ||
) : null, | ||
title: | ||
step === 1 || step === 2 | ||
? '새로운 워크스페이스 생성하기' | ||
: step === 3 | ||
? '동아리 프로필 이미지 등록' | ||
: '워크스페이스 생성완료', | ||
infoText: | ||
step === 1 | ||
? '워크스페이스의 이름을 입력해주세요' | ||
: step === 2 | ||
? '팀 카테고리를 선택해주세요' | ||
: step === 3 | ||
? '우리 동아리 프로필에 표시할 이미지를 등록해주세요' | ||
: '이제 워크스페이스를 사용할 수 있습니다.', | ||
}; |
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.
step
까지 유틸로 분리해서 처리하는 것보다는 저번에 제가 말씀드린 것처럼 step
에 따라 올바른 단계의 자식 요소를 렌더링하는 funnel
컨테이너 컴포넌트를 만들고, 해당 컴포넌트 안에서 각 모달들한테 step
prop을 넘겨주면서 그냥 그 컴포넌트 안에서 처리해도 될 것 같아요.
이거 퍼널 구조 레퍼런스 확인해보시면 좋을 것 같습니다. 토스에서 제공하는 라이브러리인 use-funnel
도 있어용
<Funnel>
<Funnel.Step step={1}>
<첫 번째 모달 />
</Funnel.Step>
<Funnel.Step step={2}>
<두 번째 모달 />
</Funnel.Step>
</Funnel>
식으로 말이죠. 즉 step
을 유틸에서 처리할 필요 없이, 그냥 컴포넌트에게 step
을 넘겨서 알아서 처리하게 말이죠 !
추가적으로 그렇다면 아래의 create-block
이나 delete
와 같은 것들은, 그냥 constant
파일에서 객체 형태로 빼줄 수도 있을 것 같아요
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.
- 각 모달에 대한 뷰만 퍼블리싱 해놓는다
- 단계에 대한 로직은
funnel
구조에게 그냥 위임해버린다 - 단계에 따른
icon
같은 것들은 그냥 상수 파일로 분리시킬 수 있다.
정도로 요약 가능하겠네요 !
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 { icon, title, infoText } = getHeaderContent(contentType!, step, totalSteps); | ||
|
||
return ( | ||
<Flex styles={{ direction: 'row', justify: 'flex-start', align: 'center', gap: '1.2rem' }}> |
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.
modalHeader니까 header 태그 사용해주면 어떨까요 ??
@@ -23,7 +19,6 @@ interface BlockModalProps { | |||
|
|||
const BlockModal = ({ isVisible }: BlockModalProps) => { | |||
const [selectedIcon, setSelectedIcon] = useState<number>(-1); |
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.
자동으로 타입 유추가 되니까 useState의 경우 타입 명시를 안해도 괜찮을 것 같아요 !
<Modal.Body> | ||
<Flex tag={'section'} css={flexStyle}> |
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.
Modal.Body 컴포넌트 자체를 section 태그로 지정하는 건 어떠신가요 ?
section태그 안에서 div로 레이아웃을 조정해주는 게 더 자연스러울 것 같다고 생각했어요
저장 | ||
</Button> | ||
</Flex> | ||
<div className="scroll" css={scrollStyle}> |
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.
여기 className를 따로 지정한 이유가 있을까요 ??
그리고 commonStyle에 scrollStyle이 이미 존재하는데 해당 스타일을 재사용할 수 있는지 궁금합니다 !
const modalData = useModalData(); | ||
|
||
const itemType = modalData?.itemType; | ||
const title = itemType ? DELETED_TITLE[itemType.toUpperCase() as keyof typeof DELETED_TITLE] : ''; |
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 title = itemType && DELETED_TITLE[itemType.toUpperCase() as keyof typeof DELETED_TITLE]
else일 떄 빈 문자열을 리턴하는 것보다 and 연산자를 통해 필요한 경우만 값을 지정하는 건 어떨까요 ? 에러가 날런가요
onDelete: () => void; | ||
} | ||
|
||
const MemberItem = ({ title, onDelete }: MemberItemProps) => { |
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.
헷갈릴 수 있다고 생각해서 폴더명과 파일명을 통일하는 게 좋을 것 같다고 생각해요!!
InviteModal 안의 컴포넌트들이니 Member 혹은 MemberItem으로 이름 지어도 Invite에 대한 의미는 전달될 것 같습니다
아니면 InviteItem이 혹시 더 생길 가능성이 있다면 InviteItem에 Member 폴더를 생성해서 그 안에 요 파일을 배치할 수도 있겠네요
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.
모달의 대격변이네요!! 피알도 정성스럽고 넘 수고하셨어요 💯
@@ -72,7 +72,7 @@ export const childrenStyle = css({ | |||
display: 'flex', | |||
alignItems: 'center', | |||
|
|||
padding: '0.4rem', | |||
//padding: '0.4rem', |
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.
요건 사용하지 않는거면 삭제해줘도 좋을 것 같네용 !
export const Modal = Object.assign(ModalWrapper, { | ||
Header: ModalHeader, | ||
Body: ModalBody, | ||
Footer: ModalFooter, | ||
}); |
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.
수고하셨습니다!! 원래도 깔끔하고 이쁜 모달이었는데 더 좋아졌네요!!!
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.
사라진 친구에게 많은 정성을 쏟아 주셨군요..!
<div className="scroll" css={scrollStyle}> | ||
{files.map((file) => ( | ||
<BlockItem | ||
key={file.name} |
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.
파일 이름은 중복될수도 있지 않을까요?!?
해당 이슈 번호
closed #281
체크리스트
📌 내가 알게 된 부분
💎 PR Point
우선 기존의 모달 뷰 였던 워크스페이스 생성하기, 타임블록 생성하기 뷰는 자세하게 안 보셔도 됩니다!! (안 봐도 됨...ㅎㅎ)( 기존의 뷰 그대로에 모달 합성 컴포넌트만 적용 함)
그리고 새로 만든 태그 모달들(ActivityTagModal, MemberTagModal), 초대 모달(InviteModal), 휴지통(DeletedModal) 모달! --> 사실 이 부분도요 대충 쓱 훑어 주시기만 해도 됩니다! 말그대로 단순! 뷰 퍼블리싱이었고 피그마에서 이 세 모달들을 보시면 아시겠지만 아이템이랑 input 위치 생김새가 굉장히 유사해요...!!
그래서 어떤 부분을 중심적으로 보시고 리뷰를 남겨주시면 좋냐...!! ==> 바로 모달 대격변한 모달 레이아웃 입니다!!
이번에 바뀐 모달은 아래 사진과 같이 세 부분으로 나누어져 있고, 모달 헤더, 모달 푸터 같은 경우는 아이콘, 텍스트, 버튼 안의 텍스트 내용만 달라지고 컴포넌트들의 구성요소들을 그대로 인것을 볼 수 있습니다!
이번에 모달 레이아웃을 바꾸면서 제일 큰 목표가 재사용성 있는 뷰 만들기 였는데요,
저는 이 반복되는 모달 레이아웃을 재사용성 있게 사용하고자 합성 컴포넌트 를 도입하였습니다.
제일 별 거 없습니다. 모달 바디안에 children 으로 컨텐츠 뷰를 주입하는 형식이기에 틀만 맞춰줬습니다.
getHeaderContent 함수??
<예외 구현 포인트>
워크 스페이스 생성 보시면 아이콘이 "완료" 모달 제외하고는 1/4 의 스텝 텍스트 표시입니다. 이를 예외처리 해주었습니다.
getFooterContent 함수??
구현 예시
워크 스페이스 생성하기 모달에서 스텝 3전, 완료 부분은 버튼이 "한개", 그 외는 두개
--> step >= 3이 아닌 경우, '건너뛰기' 버튼이 false로 설정
--> step === 4일 때는 '확인' 버튼으로, 그 외에는 '다음으로' 버튼으로 설정
요런 식으로 합성 컴포넌트 조립해서 사용하면 됩니다!!
📌스크린샷 (선택)
모달 UI 스크린샷