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: Button component #71

Merged
merged 3 commits into from
Nov 20, 2024
Merged

feat: Button component #71

merged 3 commits into from
Nov 20, 2024

Conversation

Sunjae95
Copy link
Contributor

@Sunjae95 Sunjae95 commented Nov 16, 2024

체크리스트

  • 이슈가 연결되어 있나요?

@Sunjae95 Sunjae95 self-assigned this Nov 16, 2024
@Sunjae95 Sunjae95 linked an issue Nov 16, 2024 that may be closed by this pull request
@Sunjae95 Sunjae95 marked this pull request as ready for review November 16, 2024 05:31
@Sunjae95 Sunjae95 requested a review from a team as a code owner November 16, 2024 05:31
Copy link
Contributor

@SamTheKorean SamTheKorean left a comment

Choose a reason for hiding this comment

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

버튼은 병합 후에 타 컴포넌트들을 넣으려고 하시는 걸까요?

variant: "primary" | "secondary";
block?: boolean;
size?: "small" | "large";
delay?: number;
Copy link
Contributor

@SamTheKorean SamTheKorean Nov 17, 2024

Choose a reason for hiding this comment

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

디바운싱을 위해서라면 컴포넌트내에서 값을 정의 하지 않고 컴포넌트 사용자에게 받는 이유가 궁금합니다! 혹시 버튼마다 delay가 달라질까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

버튼 마다 delay이가 존재하는건 아니에요. 제어 컴포넌트로 설계한다는 초점을 두고 설계해서 delay를 받도록 구현했어요.
이 부분은 지금 당장 없어도 문제 없는 부분이라 중요하지는 않습니다.
저는 확장할 수 있는 수준이라 판단하여 추가했는데요. @SamTheKorean 님은 컴포넌트를 설계하실 때 불필요한 항목은 빼시는 편인가요?

Copy link
Contributor

Choose a reason for hiding this comment

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

답변 감사합니다! 흠 저는 빠른 미래에도 사용 여부가 불확실한 기능을 추가하는 건 선호하지 않는 편입니다! 이 경우에는 크진 않지만 코드 복잡도가 늘어서 유지비수 비용이 늘고 컴포넌트 사용자 입장에서도 조금 혼란이 올 것 같습니다! 하지만 이 경우에는 default 값을 컴포넌트 내부에서 0으로 초기화해주셔서 큰 문제는 없을 것 같아요!

@Sunjae95
Copy link
Contributor Author

버튼은 병합 후에 타 컴포넌트들을 넣으려고 하시는 걸까요?

맞습니다. 그런데 버튼을 사용하는 곳이 certificate에만 존재하더군요...하하..

Copy link
Contributor

@SamTheKorean SamTheKorean left a comment

Choose a reason for hiding this comment

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

한 가지 코멘트를 남겼지만 큰 요소는 아닌 것 같아 승인합니다! 고생많으셨습니다!

variant: "primary" | "secondary";
block?: boolean;
size?: "small" | "large";
delay?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

답변 감사합니다! 흠 저는 빠른 미래에도 사용 여부가 불확실한 기능을 추가하는 건 선호하지 않는 편입니다! 이 경우에는 크진 않지만 코드 복잡도가 늘어서 유지비수 비용이 늘고 컴포넌트 사용자 입장에서도 조금 혼란이 올 것 같습니다! 하지만 이 경우에는 default 값을 컴포넌트 내부에서 0으로 초기화해주셔서 큰 문제는 없을 것 같아요!

@Sunjae95
Copy link
Contributor Author

한 가지 코멘트를 남겼지만 큰 요소는 아닌 것 같아 승인합니다! 고생많으셨습니다!

리뷰감사합니다. 불필요한 요소에 대해 한번 더 생각하게 된 pr이었던거 같아요!

@Sunjae95 Sunjae95 merged commit 5475aa2 into main Nov 20, 2024
2 checks passed
@Sunjae95 Sunjae95 deleted the 69-button-component branch November 20, 2024 06:15
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.

Button Component
2 participants