-
Notifications
You must be signed in to change notification settings - Fork 1
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] DefaultDialog 컴포넌트 추가 #83
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
수고하셨습니당!
inline에 있는 스타일을 어떻게 하면 좋을까요 🤔
className={css({ | ||
textStyle: 'title3', | ||
color: 'text.primary', | ||
})} |
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.
이부분 처럼 inline으로 스타일이 들어가 있는 부분은 밑으로 빼는게 어떨까요?! (위의 코드와 통일하기)
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.
넵 inline스타일들은 밑으로 빼겠습니다
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.
35e69a6 여기서 수정해주었어요
{confirmText && ( | ||
<Button size={'medium'} variant={'ghost'} onClick={handleConfirm}> | ||
{confirmText} | ||
</Button> | ||
)} |
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.
피그마를 확인해보니 취소버튼이 없는 경우는 있어도 확인 버튼은 모두 있는것으로 보입니다!
디자이너가 전달해준 디자인 시스템을 그대로 따라갈 것이라면, confirmText
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.
e31dca0 여기서 수정해주었어요
const DialogStory = (arg: Pick<DefaultDialogProps, 'title' | 'content' | 'cancelText' | 'confirmText'>) => { | ||
const { isOpen, openModal, closeModal } = useModal(); | ||
return ( | ||
<div> | ||
<Button size={'large'} variant={'cta'} onClick={openModal}> | ||
Open Dialog | ||
</Button> | ||
<Dialog variant={'default'} isOpen={isOpen} onClose={closeModal} {...arg} /> | ||
</div> | ||
); | ||
}; | ||
|
||
const meta = { | ||
title: 'Component/Dialog', | ||
component: DialogStory, | ||
parameters: { | ||
layout: 'centered', | ||
}, | ||
tags: ['autodocs'], | ||
} satisfies Meta<typeof DialogStory>; | ||
|
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 type DialogProps = DefaultDialogProps | ListDialogProps | SelectDialogProps; |
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.
진짜 사소한거지만 여기도 한칸 enter 치면 좋을 것 같아요.
이거 강제하는 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.
ee1073a 여기서 적용해주었습니다
className={css({ | ||
textStyle: 'title3', | ||
color: 'text.primary', | ||
})} | ||
> |
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.
스토리의 스타일은 그대로 두겠습니다. 저도 스토리에서는 조금 번거로울것같아요
/** | ||
* @description ref를 제외한 영역을 클릭했을 때 실행되는 hook 입니다. | ||
* @param ref | ||
* @param handler 클릭 이벤트가 발생했을 때 실행되는 함수입니다. | ||
*/ | ||
function useOutsideClick({ ref, handler }: UseOutsideClickProps) { | ||
useEffect(() => { | ||
const listener = (event: MouseEvent) => { | ||
if (!ref.current || ref.current.contains(event.target as Node)) { | ||
return; | ||
} | ||
handler(event); | ||
}; | ||
|
||
document.addEventListener('mousedown', listener); | ||
return () => { | ||
document.removeEventListener('mousedown', listener); | ||
}; | ||
}); | ||
} |
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 #76
🎉 변경 사항
🙏 여기는 꼭 봐주세요!
현재는 버튼 text와 click 핸들러를 각각 받아서 컴포넌트 안에서 실행시켜주는 방식 (확장성 낮다, 디자인가이드에 나와있는 대로 구현)
다른 방법을 고려중인 것은 확인 버튼과 취소 버튼을 element props로 받아서 랜더링 시켜주는 방식. (확장성 높지만 디자인가이드에 100% 나와있는 대로는 아니다)
버튼 컴포넌트를 props로 주입시 스토리북에서는 어떻게 보이면 좋을지도 궁금합니다.
사용 방법
🌄 스크린
샷📚 참고
https://github.com/depromeet/na-lab-client/blob/main/src/components/portal/AnimatePortal.tsx
https://www.framer.com/motion/animate-presence/