-
Notifications
You must be signed in to change notification settings - Fork 0
✨ [Feat] ChipButton 컴포넌트 제작 #8
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
base: develop
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new SwiftUI ChipButton component with environment-based configuration infrastructure. The component renders as a capsule button with selection state handling, accompanied by environment keys, ViewModifier helpers, and a size utility enum that defines three predefined button sizes with corresponding heights and fonts. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
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.
Actionable comments posted: 5
🧹 Nitpick comments (2)
AppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButton.swift (1)
35-45: Consider adding accessibility support.The button lacks accessibility labels, hints, or traits. For better user experience with VoiceOver and other assistive technologies, consider adding accessibility modifiers.
🔎 Example implementation
var body: some View { Button(action: action) { ChipButtonContent( title: title, size: size, isSelected: isSelected ) .equatable() } .buttonStyle(.plain) + .accessibilityLabel(title) + .accessibilityAddTraits(isSelected ? [.isSelected] : []) }AppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButtonModifiers.swift (1)
43-45: Add documentation for selected() modifier.The
buttonSize(_:)method has documentation (lines 37-38), butselected(_:)is missing equivalent documentation describing its purpose and parameters.🔎 Proposed documentation
+/// 버튼 선택 상태 설정 +/// - Parameter isSelected: 선택 여부 func selected(_ isSelected: Bool) -> some View { self.modifier(ChipButtonIsSelectedModifier(isSelected: isSelected)) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
AppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButton.swiftAppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButtonEnvironment.swiftAppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButtonModifiers.swiftAppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButtonSize.swift
🧰 Additional context used
🧬 Code graph analysis (1)
AppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButton.swift (1)
AppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButtonModifiers.swift (4)
body(20-22)body(28-30)selected(43-45)buttonSize(39-41)
🔇 Additional comments (4)
AppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButtonSize.swift (2)
18-27: Verify the height values are intentional.The height progression (25 → 29 → 32) uses non-standard increments. Please confirm these specific values match the design specifications.
13-16: The currentinternalaccess control (default) is appropriate for this enum.ChipButtonSizeis used exclusively within the UIComponents/ChipButton module and has no external dependencies, so addingpublicis unnecessary.AppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButton.swift (1)
55-59: ChipButtonSize automatically conforms to Equatable. The enum has no associated values, so Swift automatically synthesizes the conformance. The implementation inChipButtonContentis correct.AppProduct/AppProduct/Core/Common/UIComponents/ChipButton/ChipButtonModifiers.swift (1)
13-13: No action needed. TheAnyChipButtonprotocol's internal access control is correct and consistent with the module design. The entire ChipButton component (includingChipButtonstruct itself) is internal, andAnyChipButtonis only an internal implementation detail used for protocol conformance. Making it public while keepingChipButtoninternal would be inconsistent and provide no external value.Likely an incorrect or invalid review comment.
jwon0523
left a comment
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.
코드에 달아둔 리뷰 확인 후 수정해주세요!
| private struct ChipButtonContent: View, Equatable { | ||
| let title: String | ||
| let size: ChipButtonSize | ||
| var isSelected: Bool |
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.
isSelected는 값을 전달만 받기 때문에 let으로 선언하는게 더 나을거 같습니다.
| struct ChipButtonIsSelectedKey: EnvironmentKey { | ||
| static let defaultValue: Bool = false | ||
| } |
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.
ChipButton에서 이미 isSelected를 전달받고 있어서 해당 Environment는 불필요해 보입니다! isSelected는 사용시 필수적으로 사용될 값이니 파라미터로 두는게 더 나을거 같아요
| var chipButtonIsSelected: Bool { | ||
| get { self[ChipButtonIsSelectedKey.self] } | ||
| set { self[ChipButtonIsSelectedKey.self] = newValue } | ||
| } |
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.
위에 커멘트와 같은 이유로 제거하면 됩니다!
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.
추가로 ChipButtonModifier.swift 파일에서도 관련된 내용들을 제거해주세요!
| } | ||
| } | ||
|
|
||
| // MARK: - MainButtonContent (Presenter) |
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.
역할에 알맞게 주석 수정해주세요!
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.
버튼 이름이 잘못되어 있어요!
jwon0523
left a comment
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.
고생했어요!!
✨ PR 유형
📷 스크린샷 or 영상(UI 변경 시)
🛠️ 작업내용
커밋 히스토리
📋 추후 진행 상황
📌 리뷰 포인트
✅ Checklist
PR이 다음 요구 사항을 충족하는지 확인해주세요!!!
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.