[Feat] Button, Navigation 컴포넌트 구현 #22
Conversation
| public enum ButtonState { | ||
| case `default`(DefaultStyle) | ||
| case disable(DisableStyle) | ||
| } |
There was a problem hiding this comment.
ButtonState 대신 Style을 isEnabled: Bool과 같은 친구를 enum 연관값으로 받아서 사용하는 건 어떨까요?
case primary(isEnabled: Bool)
case gray(isEnabled: Bool)
case outline(isEnabled: Bool)
case red(isEnabled: Bool)과 같이 선언하고, 계산 프로퍼티에서 삼항 연산자로 값 줘도 좋을 것 같아요!
var backgound: Color {
switch self {
case .primary(let isEnabled):
return isEnabled ? .neutral900 : .neutral200
...
이런 느낌으로요..! 디자인 시스템이랑 1:1 매칭도 되고 좋지 않을까요?
There was a problem hiding this comment.
해당 부분 무조건 1:1 매칭되는 컴포넌트인지 한번 확인하고 해당되면 고려해보겠습니다.
There was a problem hiding this comment.
해당 내용은 모두 TButton의 UI 스타일을 규정하는 내용인 것 같은데, TButton의 Extension으로 넣으면 어떨까요?
| .padding(.horizontal, 20) | ||
| .background(backgroundColor) | ||
| .clipShape(RoundedRectangle(cornerRadius: config.radius)) | ||
| .overlay { | ||
| RoundedRectangle(cornerRadius: config.radius) | ||
| .stroke(borderColor, lineWidth: 1.5) |
There was a problem hiding this comment.
padding이나 stroke 같은 내용도 버튼 스타일 정의에 포함되면 좋을 것 같아요!
There was a problem hiding this comment.
네 앞선 리뷰와 함께 UI 스타일 관련 부분 반영해보겠습니다.
| /// TButton의 초기화 메서드 | ||
| /// - Parameters: | ||
| /// - state: 버튼의 상태 | ||
| /// - config: 버튼 구성 (크기, 글꼴 등) | ||
| /// - title: 버튼 제목 | ||
| /// - image: 버튼 이미지 (옵셔널) | ||
| /// - imageSize: 이미지 크기 (옵셔널) | ||
| /// - isEnable: 활성화 여부 (기본값: `false`) | ||
| /// - action: 버튼 탭 시 동작 (옵셔널) | ||
| public init( | ||
| state: ButtonState, | ||
| config: ButtonConfiguration, | ||
| title: String, | ||
| image: ImageResource? = nil, | ||
| imageSize: CGFloat? = nil, | ||
| isEnable: Bool = false, | ||
| action: (() -> Void)? = nil | ||
| ) { |
There was a problem hiding this comment.
init 때 한 번에 입력받는 양이 많은 것 같은데,
init때는 필수적인 값들만 챙기고, 최대한 기본 Button 선언과 비슷하게 가면서 -
밑에 정의하신 것 처럼 Modifier에서 추가 값들을 입력받는건 어떨까요?
There was a problem hiding this comment.
기존 button 처럼 최대한 title만 입력 받는 형태로 수정해보겠습니다. View 타입 자체에 추가 되는 Modifier말고 TButton 한정으로 적용하기 위해서 option action, 초기값이 있는 isEnable로 설정했어요! View 타입 자체보다는 TButton 타입에 한정해서 정의하는게 더 좋지 않을까요?
| HStack(spacing: 4) { | ||
| if let image = image, let imageSize = imageSize { | ||
| Image(image) | ||
| .resizable() | ||
| .aspectRatio(contentMode: .fit) | ||
| .frame(width: imageSize, height: imageSize) | ||
| } | ||
|
|
||
| Text(title) | ||
| .typographyStyle(config.font, with: textColor) | ||
| } |
There was a problem hiding this comment.
저희 디자인 시스템 상에서 이미지가 우측에도 있는 케이스가 있어서, 반영해주시면 좋을 것 같아요!
There was a problem hiding this comment.
좌우로 있는지 확인을 못했네요. 바로 추가해놓겠습니다!
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral900) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
| .overlay(alignment: .topLeading) { | ||
| Image(imageResource) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| } | ||
| .onTapGesture { | ||
| leftAction?() | ||
| } |
There was a problem hiding this comment.
이렇게 되면 제목 탭해도 onTapGesture가 실행되지 않을까요?!
There was a problem hiding this comment.
엇 Image에 달아야하는데 실수했네요... 수정해놓겠습니다. 감사합니다 !
| public var body: some View { | ||
| switch type { | ||
| /// 왼쪽 이미지 버튼, 센터 타이틀 | ||
| case .LButtonWithTitle(let imageResource, let title): | ||
| HStack { | ||
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral900) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
| .overlay(alignment: .topLeading) { | ||
| Image(imageResource) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| } | ||
| .onTapGesture { | ||
| leftAction?() | ||
| } | ||
| } | ||
| .padding(.init(top: 22.5, leading: 20, bottom: 10.5, trailing: 20)) | ||
|
|
||
| /// 왼쪽 이미지 버튼, 센터 타이틀, 오른쪽 이미지 버튼 | ||
| case .LRButtonWithTitle(let LImage, let title, let RImage): | ||
| HStack { | ||
| Image(LImage) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| .onTapGesture { | ||
| leftAction?() | ||
| } | ||
|
|
||
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral900) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
|
|
||
| Image(RImage) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| .onTapGesture { | ||
| rightAction?() | ||
| } | ||
| } | ||
| .padding(.init(top: 22.5, leading: 20, bottom: 10.5, trailing: 20)) | ||
|
|
||
| /// 왼쪽 이미지 버튼, 센터 타이틀, 오른쪽 텍스트 버튼 | ||
| case .LButtonRTextWithTitle(let LImage, let title, let RText): | ||
| HStack { | ||
| Image(LImage) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| .onTapGesture { | ||
| leftAction?() | ||
| } | ||
|
|
||
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral900) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
|
|
||
| Text(RText) | ||
| .typographyStyle(.body2Medium, with: .neutral400) | ||
| .onTapGesture { | ||
| rightAction?() | ||
| } | ||
| } | ||
| .padding(.init(top: 22.5, leading: 20, bottom: 10.5, trailing: 20)) | ||
|
|
||
| /// 타이틀 | ||
| case .Title(let title): | ||
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral900) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
| .padding(.init(top: 22.5, leading: 20, bottom: 10.5, trailing: 20)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 내용에서 겹치는 내용들이 있는 것 같은데,
특정 뷰만 동적으로 생성하도록 작성하면 더 명확하고 유지보수에 좋을 것 같아요..!
|
안녕하세요 🙌 잘부탁드립니다~~ |
| public var body: some View { | ||
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral50) | ||
| .padding(.vertical, 20) | ||
| .frame(maxWidth: .infinity) | ||
| .background(isEnable ? Color.neutral900 : Color.neutral300) | ||
| .ignoresSafeArea() | ||
| .onTapGesture { | ||
| if isEnable { | ||
| action?() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
버튼들 공통질문인데 왜 onTapGesture로 액션처리하게 구성하셨나요?
There was a problem hiding this comment.
단순하게 앱 내에서 사용되는 버튼이 클릭 처리라고 생각해서 onTapGeture로 충분하다고 판단했던 것 같아요. 기존에도 Text에 onTapGesture를 자주 달아 써서 Button을 활용할 생각은 안 해봤네요.. 🤔 해당 부분을 Button을 사용해서 구현하는 쪽으로 수정해보겠습니다.
| func tap(action: @escaping (() -> Void)) -> Self { | ||
| var copy: Self = self | ||
| copy.action = action | ||
| return copy | ||
| } |
There was a problem hiding this comment.
�여기도 tap은 action이랑 동일한데 선언형으로 코드 작성하고 싶어서 함수를 만드신건지 궁금합니다.
스유도 버튼 액션을 클로저로 받고있지 선언형으로 받고있지는 않잖아요 이부분 궁금합니다
There was a problem hiding this comment.
해당 부분은 선언형 코드로 작성하고 싶어서 함수를 만든게 맞습니다. 예를 들어, 네비바 같이 버튼이 한번에 2가지 있는 컴포넌트를 만들 때 클로저를 2번 사용하게 되면 가독성이 안 좋아진다고 생각해서 해당 부분을 해결하기 위해서 이 방법을 선택했어요. 근데 action이 한개인 경우는 그닥... 이렇게까지 안 해도 될것 같네요. 기존 Button을 활용한다면 더더욱 그럴것 같구요. 하나의 액션만 있는 컴포넌트는 기존 스유 방식을 반영해볼게요!
| func tap(action: @escaping (() -> Void)) -> Self { | ||
| var copy: Self = self | ||
| copy.action = action | ||
| return copy | ||
| } | ||
|
|
||
| /// 버튼 활성화 상태를 설정하는 메서드 | ||
| /// - Parameter isEnable: 활성화 여부 | ||
| /// - Returns: 업데이트된 TBottomButton | ||
| func isEnable(_ isEnable: Bool) -> Self { | ||
| var copy: Self = self | ||
| copy.isEnable = isEnable | ||
| return copy | ||
| } |
There was a problem hiding this comment.
disabled modifier가 이미 해당 함수를 대체한다고 생각하는데 새로 만드신 이유가 있을까요??
There was a problem hiding this comment.
텍스트라고 생각하다보니 따로 만들었는데 그럼에도 불구하고 굳이 필요하진 않겠군요... ? 수정해도 될것 같습니다. 감사합니다!
| /// 버튼에 표시될 이미지 (옵셔널) | ||
| public let image: ImageResource? | ||
|
|
||
| /// 이미지의 크기 (옵셔널) | ||
| public let imageSize: CGFloat? |
There was a problem hiding this comment.
서로 관련있는것들끼리 묶어서 추상화하고 이 결과를 optional로 받는 형태가 가장 이상적일 것 같아요.
하단에 image, imagesize가 둘다 있어야 이미지를 설정하는데 이렇게 생성자를 넣다보면 서로 연관되어있는지 헷갈릴 때가 많아요.
There was a problem hiding this comment.
imge, imageSize 를 한번 더 묶는게 사용성이 더 좋을 것 같다는 말씀이 맞을까요??
| /// 버튼 탭 시 수행할 동작 (옵셔널) | ||
| public var action: (() -> Void)? |
There was a problem hiding this comment.
구조체 이름이 Button인데 action이 옵셔널인 경우는 대체 어떤 케이스에서 사용되나요??
There was a problem hiding this comment.
선언형처럼 구현하기 위해서 클로저를 안 받을려고 옵셔널로 구현하였습니다. 이 부분은 기존 Button를 활용하는 방향으로 수정하면 없어질것 같아요!
| public var body: some View { | ||
| switch type { | ||
| /// 왼쪽 이미지 버튼, 센터 타이틀 | ||
| case .LButtonWithTitle(let imageResource, let title): |
| HStack { | ||
| Image(LImage) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| .onTapGesture { | ||
| leftAction?() | ||
| } | ||
|
|
||
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral900) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
|
|
||
| Image(RImage) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| .onTapGesture { | ||
| rightAction?() | ||
| } | ||
| } | ||
| .padding(.init(top: 22.5, leading: 20, bottom: 10.5, trailing: 20)) |
There was a problem hiding this comment.
case 내부에서 뷰를 그리지 마시고 함수로 빼서 외부에서 그리는건 어떠신가요??
There was a problem hiding this comment.
민서님 리뷰와 종합해서 수정 후 반영해보겠습니다-!
| public enum TNavigationCase { | ||
| /// 왼쪽 이미지 버튼, 센터 타이틀 | ||
| case LButtonWithTitle(ImageResource, String) | ||
| /// 왼쪽 이미지 버튼, 센터 타이틀, 오른쪽 이미지 버튼 | ||
| case LRButtonWithTitle(ImageResource, String, ImageResource) | ||
| /// 왼쪽 이미지 버튼, 센터 타이틀, 오른쪽 텍스트 버튼 | ||
| case LButtonRTextWithTitle(ImageResource, String, String) | ||
| /// 타이틀 | ||
| case Title(String) | ||
| } |
There was a problem hiding this comment.
네 추가하는게 좋을 것 같네요. 감사합니다!
| public var body: some View { | ||
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral50) | ||
| .padding(.vertical, 20) | ||
| .frame(maxWidth: .infinity) | ||
| .background(isEnable ? Color.neutral900 : Color.neutral300) | ||
| .ignoresSafeArea() | ||
| .onTapGesture { | ||
| if isEnable { | ||
| action?() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
단순하게 앱 내에서 사용되는 버튼이 클릭 처리라고 생각해서 onTapGeture로 충분하다고 판단했던 것 같아요. 기존에도 Text에 onTapGesture를 자주 달아 써서 Button을 활용할 생각은 안 해봤네요.. 🤔 해당 부분을 Button을 사용해서 구현하는 쪽으로 수정해보겠습니다.
| func tap(action: @escaping (() -> Void)) -> Self { | ||
| var copy: Self = self | ||
| copy.action = action | ||
| return copy | ||
| } |
There was a problem hiding this comment.
해당 부분은 선언형 코드로 작성하고 싶어서 함수를 만든게 맞습니다. 예를 들어, 네비바 같이 버튼이 한번에 2가지 있는 컴포넌트를 만들 때 클로저를 2번 사용하게 되면 가독성이 안 좋아진다고 생각해서 해당 부분을 해결하기 위해서 이 방법을 선택했어요. 근데 action이 한개인 경우는 그닥... 이렇게까지 안 해도 될것 같네요. 기존 Button을 활용한다면 더더욱 그럴것 같구요. 하나의 액션만 있는 컴포넌트는 기존 스유 방식을 반영해볼게요!
| func tap(action: @escaping (() -> Void)) -> Self { | ||
| var copy: Self = self | ||
| copy.action = action | ||
| return copy | ||
| } | ||
|
|
||
| /// 버튼 활성화 상태를 설정하는 메서드 | ||
| /// - Parameter isEnable: 활성화 여부 | ||
| /// - Returns: 업데이트된 TBottomButton | ||
| func isEnable(_ isEnable: Bool) -> Self { | ||
| var copy: Self = self | ||
| copy.isEnable = isEnable | ||
| return copy | ||
| } |
There was a problem hiding this comment.
텍스트라고 생각하다보니 따로 만들었는데 그럼에도 불구하고 굳이 필요하진 않겠군요... ? 수정해도 될것 같습니다. 감사합니다!
| /// 버튼에 표시될 이미지 (옵셔널) | ||
| public let image: ImageResource? | ||
|
|
||
| /// 이미지의 크기 (옵셔널) | ||
| public let imageSize: CGFloat? |
There was a problem hiding this comment.
imge, imageSize 를 한번 더 묶는게 사용성이 더 좋을 것 같다는 말씀이 맞을까요??
| /// 버튼 탭 시 수행할 동작 (옵셔널) | ||
| public var action: (() -> Void)? |
There was a problem hiding this comment.
선언형처럼 구현하기 위해서 클로저를 안 받을려고 옵셔널로 구현하였습니다. 이 부분은 기존 Button를 활용하는 방향으로 수정하면 없어질것 같아요!
| HStack { | ||
| Image(LImage) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| .onTapGesture { | ||
| leftAction?() | ||
| } | ||
|
|
||
| Text(title) | ||
| .typographyStyle(.heading4, with: .neutral900) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
|
|
||
| Image(RImage) | ||
| .resizable() | ||
| .frame(width: 32, height: 32) | ||
| .onTapGesture { | ||
| rightAction?() | ||
| } | ||
| } | ||
| .padding(.init(top: 22.5, leading: 20, bottom: 10.5, trailing: 20)) |
There was a problem hiding this comment.
민서님 리뷰와 종합해서 수정 후 반영해보겠습니다-!
| public enum TNavigationCase { | ||
| /// 왼쪽 이미지 버튼, 센터 타이틀 | ||
| case LButtonWithTitle(ImageResource, String) | ||
| /// 왼쪽 이미지 버튼, 센터 타이틀, 오른쪽 이미지 버튼 | ||
| case LRButtonWithTitle(ImageResource, String, ImageResource) | ||
| /// 왼쪽 이미지 버튼, 센터 타이틀, 오른쪽 텍스트 버튼 | ||
| case LButtonRTextWithTitle(ImageResource, String, String) | ||
| /// 타이틀 | ||
| case Title(String) | ||
| } |
There was a problem hiding this comment.
네 추가하는게 좋을 것 같네요. 감사합니다!
📌 What is the PR?
🪄 Changes
🌐 Common Changes
🔥 PR Point
사용법은 아래와 같습니다.
사용법은 아래와 같습니다.
📸 Screenshot
🙆🏻 To Reviewers
💭 Related Issues