-
Notifications
You must be signed in to change notification settings - Fork 2
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 [#9] 커스텀 버튼 구현 #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P4. 고생하셨습니다 ◡̈ 👍
self.do { | ||
$0.makeCornerRound(radius: 6.adjustedHeight) | ||
$0.layer.cornerCurve = .continuous | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P5. 여기서는 굳이 이렇게 do를 사용할 필요없이 self.makeCornerRound
이런 식으로 작성해도 괜찮을 것 같은데 어떻게 생각하시나요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P4. 또한 이런 경우에는 @frozen enum
으로 만드는 것보다는 enum으로 만든 뒤, default case를 사용하는 건 어떤가요?
default case를 이용해서 switch문 하나로 모두 처리할 수 있을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀해주신 리뷰대로 두 개의 switch 문을 하나의 switch문으로 변경하였습니다!
하지만 역시 @Frozen enum으로 사용할 수 있어보이는데 default case로 분기하는 것이 어떤 것을 의미하나요?
HMH_iOS/HMH_iOS/Presentation/Common/UIComponets/HMHSelectButton.swift
Outdated
Show resolved
Hide resolved
self.do { | ||
$0.makeCornerRound(radius: 6.adjustedHeight) | ||
$0.layer.cornerCurve = .continuous | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P4. 여기도 위와 마찬가지로 @frozen
을 제거하고 switch문으로 바꾸는 건 어떠신가요 ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p5. 오,, 너무 고생하셨어요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P5. 적극적인 반영 감사합니다요 ~ 🤝
self.do { | ||
$0.isEnabled = true | ||
$0.backgroundColor = .bluePurpleButton | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P5. 여기도 위랑 똑같이 self.isEnabled = true
이런 식으로 작성 가능 할 것 같아요!
final class OnboardingButton: UIButton { | ||
@frozen | ||
enum OnboardingButtonType { | ||
case enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P5. 소소하지만 .. case naming 혹시 enabled
는 어떠신가요? disabled
와 맞추는 게 좋아보여서요!
👾 작업 내용
OnboardingButton
HMHSelectButton
🚀 PR Point
📸 스크린샷
✅ Issue
Resolved #9