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

common modal component/#38 #40

Merged
merged 5 commits into from
Mar 4, 2024
Merged

common modal component/#38 #40

merged 5 commits into from
Mar 4, 2024

Conversation

yougyung
Copy link
Member

@yougyung yougyung commented Mar 1, 2024

📌 작업 내용

구현 내용 및 작업 했던 내역

  • useModal 구현
  • Modal component 구현
  • story book test

🤔 고민 했던 부분

  • 고민했던 포인트가 없다면 삭제해도 됩니다.

  • 어떤 고민을 해서 어떤 결과가 나왔는지 작성해주세요.

  • 경험 공유나 다른 팀원들의 생각을 들을 수 있을 것 같아요.

  • modal compoent가 atom component인가에 대한 고민을 진행했는데 modal을 이루는 content가 없더라도 modal 그 자체로 component를 이루고 있으며, 이는 더 이상 분리할 수 없다는 차원에서 atom component로 개발하였습니다.

  • modal이 별도로 title 등의 형식을 갖지 않도록 개발했습니다.

    • 저희 서비스에서 모달이 정해진 형식을 갖지 않고 자유롭게 사용되고 있다는 차원에서 양식을 갖추는 것이 개발을 제한하는 것이라는 생각이 들어 별로의 형식을 갖지 않습니다.
  • 각 컴포넌트에 대한 모달이므로 각 컴포넌트에서 모달을 제어(visible, toggle)하도록 useModal을 생성하여 호출을 위임했습니다.

  • 컴포넌트의 사이즈를 별도로 지정하지 않았으며 max, min 값만 지정하였습니다.

    • 해당 기준은 이전 프로젝트의 기준을 적용했으며, 아래 목록 리스트입니다.
    •     max-width: 90%;
          min-width: 30%;
          overflow-y: auto;
          max-height: 70vh;  
          max-height: 90vh !important; //pc기준 
      
  • 호출부 예시

  •      const { toggle, visible } = useModal();
    
          <Modal open={visible} onOpenChange={toggle}><SignUpModal/></Modal>
          <button onClick={toggle}>회원가입하기</button>

🔊 도움이 필요한 부분

  • 도움이 필요한 부분이 없다면 삭제 해도 됩니다.

  • 팀원들의 의견이 꼭 필요한 부분을 작성해주세요.

  • 팀원들은 놓치지 않고 꼭 이 항목을 보고 의견을 주세요.

  • 피드백 주시면 적극 반영하겠습니다.

@yougyung yougyung linked an issue Mar 1, 2024 that may be closed by this pull request
@yougyung yougyung self-assigned this Mar 1, 2024
Copy link

github-actions bot commented Mar 1, 2024

Copy link
Member

@gahyuun gahyuun left a comment

Choose a reason for hiding this comment

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

더 이상 분리할 수 없다는 차원에서 atom component로 개발하셨는데 저는 modal 컴포넌트가 molecule 또는 organism 단위라고 생각해요!

  • 현재 구현하신 모달 컴포넌트 또한 root, portal, content 등 여러 컴포넌트로 이루어진 복합체라고 생각합니다! 그래서 더 이상 분리할 수 없다고 보기엔 어려울 것 같습니다
  • modal 은 기능을 수행합니다

'outline-none fixed left-[50%] top-[50%] zIndex-3 max-w-[90%] min-w-[30%] overflow-y-auto translate-x-[-50%] translate-y-[-50%] bg-white p-6 shadow-lg duration-200 rounded-lg max-h-[70vh] lg:max-h-[90vh]',
noneSlideAnimation,
fadeAnimation,
className,
Copy link
Member

Choose a reason for hiding this comment

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

className 을 props로 전달 받는 이유가 따로 있나용?

Copy link
Member Author

Choose a reason for hiding this comment

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

width나 height의 값을 children으로 전달되는 component에서 제어하고 싶을 가능성을 염두하여 추가했습니다. 현재 상태에서는 별도로 사용처가 예상되지않는데, 불필요하다고 여겨진다면 수정하겠습니다 !

Copy link
Member

Choose a reason for hiding this comment

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

className이 꼭 필요한 상황이 아니면 없는 게 더 좋을 것 같아요..!
저희 서비스에 맞게 공통 컴포넌트를 구현하는 것이므로 className을 통해 확장성을 부여할 필요가 없을 것 같네요!

Copy link
Member

Choose a reason for hiding this comment

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

Modal 창 외부를 클릭했을 때 모달이 닫히는 기능도 있으면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

현재 구현되어져있습니다:)

Copy link
Member

Choose a reason for hiding this comment

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

앗 스토리북만 확인해서 구현이 안된 줄 알았네요!

@@ -0,0 +1,36 @@
'use client';

import { Portal, Overlay, Content, Root } from '@radix-ui/react-dialog';
Copy link
Member

Choose a reason for hiding this comment

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

radix에서 제공하는 dialog trigger도 유용해보이는데 따로 사용 안하시는 이유가 있을까용?

Copy link
Member Author

Choose a reason for hiding this comment

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

상태를 외부에서 주입하고 관리하도록 명확한 분리를 목적으로 useModal을 사용했습니다:)

Copy link
Collaborator

@seonghunYang seonghunYang left a comment

Choose a reason for hiding this comment

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

  • 확인했습니다. 어제 새벽에 갑자기 일이 생겨서 늦게 확인하게 되었습니다. 미안합니다.
  • 모달 컴포넌트에 대한 의견을 말씀드리면, 제 생각에는 아토믹 디자인 시스템은 원래 디자인 시스템 즉, 디자이너의 관점에서 개발자와의 커뮤니케이션 도구이므로, 코드 레벨보다는 시각적 레벨에서 분리되어야 한다고 생각합니다.
  • 말씀하신대로 모달은 코드 레벨에서는 독립된 컴포넌트이지만, 시각적 레벨에서는 다양한 컴포넌트와 함께 구성되므로, 아토믹 디자인 시스템에서는 molecule이 더 적합하다고 생각합니다.

@yougyung yougyung requested review from seonghunYang and gahyuun March 2, 2024 03:01
Copy link

github-actions bot commented Mar 2, 2024

seonghunYang
seonghunYang previously approved these changes Mar 2, 2024
Copy link
Collaborator

@seonghunYang seonghunYang left a comment

Choose a reason for hiding this comment

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

  • 수고하셨습니다

Copy link

github-actions bot commented Mar 2, 2024

Copy link
Member

@gahyuun gahyuun left a comment

Choose a reason for hiding this comment

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

확인했습니다! 고생하셨습니다

@yougyung yougyung requested a review from seonghunYang March 3, 2024 01:12
@yougyung yougyung merged commit 62d606f into main Mar 4, 2024
2 of 3 checks passed
@yougyung yougyung deleted the modal/#38 branch March 4, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

공통 모달 컴포넌트 생성
3 participants