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: Dialog 컴포넌트 구현 #19

Merged
merged 4 commits into from
Jan 28, 2024
Merged

feat: Dialog 컴포넌트 구현 #19

merged 4 commits into from
Jan 28, 2024

Conversation

Puterism
Copy link
Member

  • Dialog 컴포넌트 및 useDialogState 커스텀 훅 구현
    • 스크린샷 2024-01-27 오후 11 59 52
    • useDialogState 커스텀 훅은 Dialog 상태 관리 코드를 별도로 작성하지 않고 사용하게끔한 유틸성 커스텀 훅임. 써도 되고 안 써도 됨.
    • HomePage에 예시 코드 작성해둠.

@Puterism Puterism self-assigned this Jan 27, 2024
Copy link

netlify bot commented Jan 27, 2024

Deploy Preview for boolti-super-admin ready!

Name Link
🔨 Latest commit f013caa
🔍 Latest deploy log https://app.netlify.com/sites/boolti-super-admin/deploys/65b61743fedc1a00080ba74d
😎 Deploy Preview https://deploy-preview-19--boolti-super-admin.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 27, 2024

Deploy Preview for boolti-preview ready!

Name Link
🔨 Latest commit f013caa
🔍 Latest deploy log https://app.netlify.com/sites/boolti-preview/deploys/65b6174367ecc40008112da8
😎 Deploy Preview https://deploy-preview-19--boolti-preview.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 27, 2024

Deploy Preview for boolti-admin ready!

Name Link
🔨 Latest commit f013caa
🔍 Latest deploy log https://app.netlify.com/sites/boolti-admin/deploys/65b617436f90960008d83d1c
😎 Deploy Preview https://deploy-preview-19--boolti-admin.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -0,0 +1,27 @@
const CloseIcon = () => (
Copy link
Member

Choose a reason for hiding this comment

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

아이콘은 나중에 모아서 svgr 넣어서 파일로 만들거나.. 아이콘 폴더 따로 만들어야겠다!

const DimmedArea = styled.div`
position: fixed;
inset: 0;
background-color: rgba(86, 86, 86, 0.25);
Copy link
Member

Choose a reason for hiding this comment

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

하드코딩하는 색상은 최대한 피하는게 좋을 듯!? dim.dialog 같이 팔레트에 넣어서 써보는건 어떨까 싶어!
한번 설정하면 잘 바뀌지는 않겠지만, 나중에 찾기 어려울수도 있으니..!

Copy link
Member Author

Choose a reason for hiding this comment

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

6eaedd5 에서 반영 완료

import React from 'react';
import ReactDOM from 'react-dom';

interface PortalProps {
Copy link
Member

Choose a reason for hiding this comment

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

id같은 것을 추가로 받아서 이미 만들어졌으면 사용하고 없으면 새로 생성하는 방식으로 써도 좋겠다!
모달, 다이얼로그 등등.. 혹시나 포탈 쓰는 것들이 많아지면 필요할 것 같아!

Copy link
Member Author

@Puterism Puterism Jan 28, 2024

Choose a reason for hiding this comment

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

포탈에 id를 달아야 하는 이유가 있을까? id로 뭔가 관리를 해야하는 경우가 생길까?

Copy link
Member

Choose a reason for hiding this comment

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

포탈 쓰는 이유가 계층 내부에수 선택자 충돌에 대비한거니까, 모달, 다이얼로그 같은 컴포넌트는 각각의 포탈을 갖는게 맞다고 생각했어! (사실 크게 중요치는 않을 듯)

Copy link
Member Author

Choose a reason for hiding this comment

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

내가 Portal에 대해서 잘 모르고 하는 이야기일 수도 있지만, Portal을 생성하는데 많은 비용이 들지는 않을 거라고 생각해서 요건 이슈가 생기면 달아줘도 좋을 것 같다고 생각해!
현재 Dialog 컴포넌트도 각각 Portal을 갖도록 구현하기도 했으니까!

packages/ui/src/hooks/useDialogState.ts Outdated Show resolved Hide resolved
apps/admin/src/pages/HomePage/HomePage.tsx Outdated Show resolved Hide resolved
@Puterism Puterism merged commit 0107f76 into main Jan 28, 2024
13 checks passed
@Puterism Puterism deleted the feature/dialog branch January 28, 2024 09:19
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.

2 participants