-
Notifications
You must be signed in to change notification settings - Fork 0
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: 공통 모달 컴포넌트 구현 및 약관 동의 UI 구현 #17
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.
고생하셨습니다! 제가 말씀드린 리뷰는 나중에 시간날때 수정하거나 현재 방식이 더 나으면 그대로 진행해도 좋을 것 같아요!
</Button> | ||
</div> | ||
<div className="flex items-center justify-between"> | ||
<Checkbox |
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.
약관 항목들이 거의 동일한 구조로 반복되고 있어서, 배열과 map
을 활용하면 더 간결한 코드가 될 수 있을 것 같은데 나중에 시간될때 한 번 생각해보시면 좋을 것 같아요!
const agreementItems = [
{ name: 'termsAgreement', label: '이용 약관 동의ㅣ(필수)' },
{ name: 'privacyAgreement', label: '개인정보 수집 및 이용 동의(필수)' },
{ name: 'marketingAgreement', label: '광고성 정보 수신 동의(선택)' },
];
{agreementItems.map((item) => (
<div key={item.name} className="flex items-center justify-between">
<Checkbox
control={agreementForm.control}
name={item.name as keyof z.infer<typeof agreementsSchema>}
// 이 부분은 이렇게 적는지는 잘 모르겠지만... 요론식으로
label={item.label}
/>
<Button variant="icon">
<AgreeLinkIcon />
</Button>
</div>
))}
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.
좋은 방법인 것 같습니다!! 적용해볼게요 😁
src/components/modal/index.tsx
Outdated
); | ||
}; | ||
|
||
Modal.Header = ({ |
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.Header
,Modal.Content
,Modal.Footer
가 반복적인 구조를 갖고 있어서
const ModalSection = ({
children,
className,
}: {
children: React.ReactNode;
className?: string;
}) => (
<div className={`w-full h-fit mb=vertical ${className || ''}`}>
{children}
</div>
);
Modal.Header = ModalSection;
Modal.Content = ModalSection;
Modal.Footer = ModalSection;
이런식으로 적용해도 좋을 것 같아요!
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.
작성할 때 모달이 확장되면 추가적으로 변경점이 있을수도 있겠다 싶어서 따로 빼두긴 했는데.. 뭔가 모달을 여기서말고는 사용을 안할거같아서 그냥 말씀대로 합치는게 나을 것 같아요 😂`
); | ||
}; | ||
|
||
// eslint-disable-next-line react-refresh/only-export-components |
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.
요기서 eslint에 걸리는 부분은 어떤 부분인가요,,?_?
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 해주는 함수가 여러개일 경우에 한 페이지 내에서 작성하지 말라는건데.. 보통 context
작성할 때 이런 로직으로 많이 사용하거든요...? 모달 사용해줄 provider 랑 모달 상태 제어할 함수들 export 해주는거라 다른 파일에 각각 작성하는건 좋지 않을 것 같아서 이런 식으로 사용했던건데 뭔가 애매하네요.. 😞
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.
아항... 그럼 저 부분은 일단 eslint 무시하는 방향으로 가는게 좋겠네요!
1. 무슨 이유로 코드를 변경했나요?
2. 어떤 위험이나 장애를 발견했나요?
3. 관련 스크린샷을 첨부해주세요.
4. 완료 사항
공통 모달 컴포넌트와 약관 동의 모달 UI 구현했습니다!
모달 애니메이션이나 확장 등은 이후의 브랜치에서 작업할 예정입니다!!
아마 모달을 사용하는게 현재로선 약관 동의와 매칭 부분 두개일 것 같긴한데.. 그래도 공통으로 빼는게 재사용성 측면에서 나은 선택이 될 것 같아서 이렇게 진행했습니다!
사용하실 땐
useModal
임포트 하셔서openModal
함수 가져오시고와 같이 사용하시면 됩니다!
5. 추가 사항
index.html
에 모달 렌더링용root
를 하나 추가해뒀는데 이런 식으로 하면 모달의 독립성 향상과 더불어 내부 스타일링 충돌을 방지할 수 있다고 해서 적용해보았습니다! 피드백은 언제든 환영입니다 😁그리고 아마 저희 서비스 자체가 데스크톱에서의 사용은 거의 없을 것..? 같아서 일단 모달을 Escape로 닫는 로직은 구현하지 않았습니다!
나중에 필요 시에 추가하겠습니다!!
또 요번 PR에서 디자인 변경된 부분들도 최신화했다보니 주제와 관련없는 변경점은 넘어가셔도 될 듯 하고 모달 구현 부분만 봐주시면 될 것 같습니다!!