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

[Design System] Alert, Confirm, Prompt #102

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

rlawdgus
Copy link
Contributor

@rlawdgus rlawdgus commented Feb 3, 2023

개요 🔍

  • Alert, Confirm, Prompt 컴포넌트 개발 PR입니다.

작업 내용 📝

  • 디자인을 입힌 알림창 컴포넌트를 사용할 수 있게 개발 중입니다.
  • 컴포넌트, 컨텍스트 두 가지 형태로 제공할 계획입니다.

기타 사항 🙋‍♂️

  • 23.02.03 기준 완성 후 PR이 아니고 컨텍스트 형태인 알림창의 사용성 및 안정성 등을 검토하기 위함입니다.
  • 검토 전 Alert, AlertProvider만 구성했고 나머지 컴포넌트들과 각각 테스트 및 storybook은 계속 작업하고 있겠습니다.
  • 이 컴포넌트에서 사용한 버튼 및 입력창은 해당 컴포넌트의 완성에 따라 업데이트 하겠습니다.

@rlawdgus rlawdgus added Developer 개발자 이슈 Process 작업 단위(디자인 + 개발) labels Feb 3, 2023
@rlawdgus rlawdgus added this to the Atom Component milestone Feb 3, 2023
@rlawdgus rlawdgus self-assigned this Feb 3, 2023
@netlify
Copy link

netlify bot commented Feb 3, 2023

Deploy Preview for ids-storybook ready!

Name Link
🔨 Latest commit d42faac
🔍 Latest deploy log https://app.netlify.com/sites/ids-storybook/deploys/63dcb25253f5db0009d38c75
😎 Deploy Preview https://deploy-preview-102--ids-storybook.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 settings.

Copy link
Collaborator

@IRONAGE-Park IRONAGE-Park left a comment

Choose a reason for hiding this comment

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

Context로 사용하는 방식의 장점이 아직 와닿지 않는 것 같습니다.
일단, 사용자의 입장에서 Alert 컴포넌트를 위해 Provider를 사용해야 한다는 점과, onConfirm이 선언된 이후 반드시 Alert가 닫히게 되어 중간 작업이 필요할 경우(Alert를 중첩해서 띄운다거나)에 확장성이 닫혀있지 않을까 생각이 듭니다.

background: ${({ theme }) => theme.color.primary.blue500};
border: none;
border-radius: 5px;
color: white;
Copy link
Collaborator

Choose a reason for hiding this comment

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

컬러 코드는 디자인 시스템으로 통일하는게 좋을 것 같습니다.

description: '',
confirmButtonLabel: '확인',
// eslint-disable-next-line @typescript-eslint/no-empty-function
onConfirm: () => {},
Copy link
Collaborator

Choose a reason for hiding this comment

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

어차피 옵셔널 인자라면 반드시 초기 상태로 넣어줄 필요는 없을 것 같습니다.

alert.onConfirm();
}

setAlert((prev) => ({ ...prev, open: false }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 경우에, 만약 이전 상태에서 description, confirmButtonLabel 등의 옵셔널 인자를 초기화 해주지 않으면, 해당 내용을 이후에 변경해주지 않고 사용할 때 이전 기록이 남아있을 것 같습니다.

const openAlert = useContext(AlertContext);

return useCallback(
(title: string, description: string, onConfirm?: () => void, confirmButtonLabel?: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

옵셔널 인자들은 순서에 영향을 받지 않도록 객체로 묶어서 사용하면 좋을 것 같습니다.
만약 onClose 등의 함수가 추가된다면 모든 코드에서 순서를 변경해야 하므로 어려움이 발생할 것 같네요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer 개발자 이슈 Process 작업 단위(디자인 + 개발)
Projects
None yet
2 participants