Skip to content
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(web-domains): 공통 모달 컴포넌트 #71

Merged
merged 5 commits into from
Jul 26, 2024
Merged

Conversation

Andrevile
Copy link
Member

@Andrevile Andrevile commented Jul 26, 2024

🎉 변경 사항

🔗 링크

🙏 여기는 꼭 봐주세요!

export const ArrivedQuestionNotification = ({ ...rest }: ArrivedQuestionNotificationProps) => {
  const [isOpen, setIsOpen] = useState<boolean>(false);

  const handleModalClose = () => {
    setIsOpen(false);
  };

  return (
    <Modal {...rest} isOpen={isOpen} onClose={handleModalClose}>
      <div></div>
    </Modal>
  );
};
  • isOpen 이 true 면 모달 노출
  • onClose 함수를 넘겨줘서 isOpen을 false로 변경시키게 함
  • children으로 내용물 커스텀하게 할 수 있음
  • footer영역도 추가 가능

@Doeunnkimm
Copy link
Member

@Andrevile 어떻게 사용하는지 PR 설명에 함 적어주실 수 있나영 🙇🏻‍♀️

@Andrevile
Copy link
Member Author

@Doeunnkimm
적었습니다 한번 참고해주세여

semnil5202
semnil5202 previously approved these changes Jul 26, 2024
Copy link
Collaborator

@semnil5202 semnil5202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다~

transform: 'translate(-50%, -50%)',
backgroundColor: colors.white,
borderRadius: borderRadiusVariants.medium,
zIndex: '10000',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 createPortal 사용하면 zIndex 필요 없지 않나요? 🤔

const [modalState, setIsModalState] = useState<boolean>(isOpen);
const [element, setElement] = useState<HTMLElement | null>(null);

const modalWidth = width ? `${width}px` : '100%';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모바일 뷰라서 너비를 만질 일이 많이 없을 것 같은데
prop에서 width 제거하고, 내부에서는 제일 많이 사용하는 너비로 width 고정적으로 스타일링하는 것 어떠신가용 ? (maxWidth === 기본 width로..?)

진짜 만약 필요하면 사용처에서 스타일 오버라이드 하라고 하죠 ~!

Comment on lines 39 to 49
if (typeof window === 'undefined') {
return <></>;
}

if (!element) {
return <></>;
}

if (!isOpen) {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 조건 싸그리 묶어서 그냥 얼리 리턴 해도 될듯..? (리팩터링 느낌으로..)
어차피 모달 렌더링 안 하겠다 의미 맞죵 ?

@Andrevile Andrevile force-pushed the feat/component-modal branch from f04c84e to 69893bb Compare July 26, 2024 16:15
@Doeunnkimm Doeunnkimm changed the title feat: 공통 모달 컴포넌트 feat(web-domains): 공통 모달 컴포넌트 Jul 26, 2024
Copy link
Member

@Doeunnkimm Doeunnkimm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@Andrevile Andrevile merged commit 092c6c0 into main Jul 26, 2024
2 of 3 checks passed
@Andrevile Andrevile deleted the feat/component-modal branch July 26, 2024 16:18
@semnil5202
Copy link
Collaborator

요 모달 isOpen의 초기값이 false면 true로 변경이 안되네여~~

즉 처음부터 모달을 띄우는 상황이 아닌, 어떤 요소를 클릭해서 모달을 띄우고자 할 때 사용이 불가합니다 ㅠ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants