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

fix: 로그인 버튼 레이아웃을 수정해요 #664

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

akrudal
Copy link
Collaborator

@akrudal akrudal commented Sep 29, 2024

😽개요

  • 로그인 버튼 레이아웃을 수정해요 :)

🛠️작업 내용

BBButton에 ImageView 추가

  • 원래 버튼에 image 다음 title이 들어가게 되어있어서 inset으로 조정이 가능한데, BBButton같은 경우에는 titleLabel을 추가한 것이라서 동일하게 imageView를 추가하여 사용하였습니다! @rlarjsdn3 이부분 봐주세용

버튼 레이아웃 수정

  • 이외에는 이미지로 되어있던 카카오, 애플 로그인 버튼을 BBButton으로 수정한 부분입니당

✅테스트 케이스

  • 기존 버튼 레이아웃에 문제가 없는지 체크하였습니당!

@akrudal akrudal self-assigned this Sep 29, 2024
@akrudal akrudal linked an issue Sep 29, 2024 that may be closed by this pull request
1 task
Copy link
Collaborator

@rlarjsdn3 rlarjsdn3 left a comment

Choose a reason for hiding this comment

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

고생하셨어여

@@ -100,6 +107,14 @@ public class BBButton: UIButton {
self.id = id
}

public func setImageWithTitle(image: UIImage?, title: String?) {
Copy link
Collaborator

@rlarjsdn3 rlarjsdn3 Sep 29, 2024

Choose a reason for hiding this comment

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

BBButton을 상속받아 BBImageButton 클래스를 하나 따로 만들어서 구현하시는 게 어때요? BBButton 하나에 너무 많은 일을 하는 느낌이 들어서 그런데 한번 고려해주세요.

메서드 이름을 setTitle(image: UIImage?, title: String?, state: UIControl.State)로 오버로딩해주실 수 있을까요? 메서드 이름이 너무 길어보이기도 하고 의미 전달이 충분하리라 생각이 들어요.

문서 주석도 한 줄 정도 달아주실 수 있나요? 예전에 구현하신 setBackground() 메서드도 부탁드려요.

Copy link
Collaborator Author

@akrudal akrudal Sep 29, 2024

Choose a reason for hiding this comment

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

음 저도 개인적인 생각으로 답변 달아볼게여..!!

버튼 하나에 너무 많은 일을 한다기에는
버튼은 원래 이미지 버튼, 이미지 텍스트 버튼, 텍스트 버튼이 있습니다.

이거를 저희는 UIButton 하나로 쓰고요
제 생각에는 BBButton이 너무 많은 일을 하고 있다 생각하지 않습니다.

결국 하는 일은 버튼이라는건데, 이 버튼 하는 역할을 모아둔게 클래스라고 생각해요
이거를 단순히 UI상의 문제로 이미지와 이미지 없는 버전을 나눠야한다 생각하지 않습니다!

예를 들어서, 버튼에 텍스트만 있다가, 선택하면 이미지 텍스트로 바뀌어야한다 이러면 BBButton이 두 개 수행해야하지 않을까욤?! 이런 이유로 저는 UI의 자유는 줬으면 좋겠습니다!
만약 image에는 뭐 클릭이 안되게하거나, 하이라이트가 안먹게 하거나 이런 기능상의 차이가 있으면 상속 받는 클래스를 만드는게 맞다 생각해요!

메서드 이름같은 경우에도 정확한게 좋다고 생각하는데,
setImage로 해서 title하고는 분리하겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

네 이해했어요~ 문서 주석만 잘 달아주세요

@rlarjsdn3 rlarjsdn3 self-requested a review September 29, 2024 14:00
rlarjsdn3

This comment was marked as resolved.

rlarjsdn3

This comment was marked as resolved.

Do-hyun-Kim
Do-hyun-Kim previously approved these changes Sep 29, 2024
@@ -14,7 +14,8 @@ import SnapKit
public class BBButton: UIButton {

// MARK: - Views

public var mainStackView: UIStackView = UIStackView()
public var mainImageView: UIImageView = UIImageView()
Copy link
Collaborator

Choose a reason for hiding this comment

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

mainStackView , mainStackView 같은 경우에는 setImageWithTitle 메서드로 setter 작업을 할 수 있어서 private 키워드를 사용하는 게 좋을 것 같은데 어떠신가염??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋아여

@akrudal akrudal dismissed stale reviews from Do-hyun-Kim and rlarjsdn3 via f12f971 September 30, 2024 04:26
@akrudal akrudal merged commit 3ef5910 into develop Sep 30, 2024
0 of 2 checks passed
@akrudal akrudal deleted the fix/#623-signIn-button-layout branch September 30, 2024 04:27
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.

fix: 로그인 버튼 레이아웃을 수정합니다.
3 participants