-
Notifications
You must be signed in to change notification settings - Fork 0
✨ [Feat] LoadingView 컴포넌트 구현 #10
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
Conversation
📝 WalkthroughWalkthroughIntroduces a new LoadingView SwiftUI component system consisting of a generic overlay wrapper, environment configuration for presentation state and messaging, view modifiers for fluent API chaining, and a size enumeration with predefined dimensions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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: 1
🧹 Nitpick comments (3)
AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewEnvironment.swift (1)
10-41: Well-structured environment key definitions.The implementation follows SwiftUI's standard pattern for custom environment values. The separation into distinct keys for size, presentation state, and message provides flexibility.
Consider using
LocalizedStringKeyorString(localized:)for the default message if localization is planned:struct LoadingViewMessageKey: EnvironmentKey { - static let defaultValue: String = "Loading..." + static let defaultValue: String = String(localized: "Loading...") }AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingView.swift (2)
51-62: Minor formatting issue and UX consideration.Line 55 has an extra space before the comma. Additionally, consider adding a semi-transparent background overlay to prevent user interaction with content during loading:
🔎 Proposed improvements
var body: some View { VStack(spacing: 10) { ProgressView() .progressViewStyle(.circular) - .frame(width: size.size.width , height: size.size.height) + .frame(width: size.size.width, height: size.size.height) if let message { Text(message) .font(size.font) } } + .frame(maxWidth: .infinity, maxHeight: .infinity) + .background(Color.black.opacity(0.3)) }
72-81: Preview demonstrates basic usage.Consider adding additional preview variants to showcase different configurations (e.g., small size, no message, custom message):
#Preview("Loading Small") { LoadingView { Text("Content") } .loadingSize(.small) .loadingMessage("Please wait...") .presented(.constant(true)) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingView.swiftAppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewEnvironment.swiftAppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewModifier.swiftAppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewSize.swift
🧰 Additional context used
🧬 Code graph analysis (2)
AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewSize.swift (1)
AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewModifier.swift (3)
body(22-24)body(30-32)body(38-40)
AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingView.swift (1)
AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewModifier.swift (4)
body(22-24)body(30-32)body(38-40)presented(54-56)
🔇 Additional comments (3)
AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingView.swift (1)
12-39: Good component structure with environment-based configuration.The use of
@Environmentfor reactive state propagation and theEquatableconformance onLoadingViewContentfor render optimization are well-implemented SwiftUI patterns.AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewModifier.swift (1)
19-41: Clean ViewModifier implementations.The modifiers correctly propagate values to the SwiftUI environment. The use of
@BindinginLoadingViewIsPresentedModifierallows for two-way binding if needed in the future.AppProduct/AppProduct/Core/Common/UIComponents/LoadingView/LoadingViewSize.swift (1)
10-34: Clean enum design for loading view sizing.The enum provides a scalable structure for size and typography configuration. Since enums without associated values are implicitly
Equatablein Swift, the comparison inLoadingViewContentwill work correctly. The custom.app(_:weight:)font extension is properly defined in the design system tokens.
| extension AnyLoadingView { | ||
|
|
||
| /// 로딩 사이즈 설정 | ||
| /// - Parameter size: small, large | ||
| func loadingSize(_ size: LoadingViewSize) -> some View { | ||
| self.modifier(LoadingViewSizeModifier(size: size)) | ||
| } | ||
|
|
||
| /// 로딩 isPresented | ||
| func presented(_ isPresented: Binding<Bool>) -> some View { | ||
| self.modifier(LoadingViewIsPresentedModifier(isPresented: isPresented)) | ||
| } | ||
|
|
||
| /// 로딩 메시지 설정 | ||
| func loadingMessage(_ message: String) -> some View { | ||
| self.modifier(LoadingViewMessageModifier(message: message)) | ||
| } |
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.
Modifier chaining limitation due to return type.
The modifiers return some View instead of preserving the AnyLoadingView conformance, which breaks fluent API chaining. After calling one modifier, subsequent AnyLoadingView-specific modifiers won't be available.
For example, this won't compile:
LoadingView { Text("Content") }
.loadingSize(.small)
.presented(.constant(true)) // Error: 'some View' has no member 'presented'Consider one of these solutions:
🔎 Option 1: Move modifiers to a View extension (simpler)
-extension AnyLoadingView {
+extension View {
/// 로딩 사이즈 설정
/// - Parameter size: small, large
func loadingSize(_ size: LoadingViewSize) -> some View {
- self.modifier(LoadingViewSizeModifier(size: size))
+ modifier(LoadingViewSizeModifier(size: size))
}
/// 로딩 isPresented
func presented(_ isPresented: Binding<Bool>) -> some View {
- self.modifier(LoadingViewIsPresentedModifier(isPresented: isPresented))
+ modifier(LoadingViewIsPresentedModifier(isPresented: isPresented))
}
/// 로딩 메시지 설정
func loadingMessage(_ message: String) -> some View {
- self.modifier(LoadingViewMessageModifier(message: message))
+ modifier(LoadingViewMessageModifier(message: message))
}
}🔎 Option 2: Use wrapper types to preserve AnyLoadingView conformance
This approach is more complex but preserves the type constraint if that's important for your API design.
|
@LeeYeJi546 화면 전체를 감싸는 로딩 뷰를 생각하고 만든거라면, overlay 방식으로 처리할 수 있도록 해봐요 사용할 때 마다 로딩 텍스트를 입력하고 등장 시키는게 아닌, 로딩 뷰가 앱 전체에서 사용될 모든 로딩 텍스트를 관리할 수 있도록 내부에 enum을 배치하고, 외부에서는 case를 선택해서 사용만 할 수 있도록 하는거죠 또 프로그레스 뷰의 size 조절은 controlSize로 하는거요! frame으로 size 조절이 아닙니다! 해당 내용을 잘 적용해서 답에 가깝게 수정이 안되고, pr로 또 올라온다면 그냥 제가 바꿔두도록 할게요! |
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.
코드에 달아둔 리뷰 확인해서 수정해주세요!
제웅이 리뷰한 내용과 함께 확인해서 반영해주면 됩니당
| struct LoadingViewMessageKey: EnvironmentKey { | ||
| static let defaultValue: String = "Loading..." | ||
| } |
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.
LodingView.swift에서 43번째 줄에서는 옵셔널로 받고 있는데, 해당 키에서 기본값으로 'Loading...'으로 명시해두면 LodingView.swift에서 옵셔널로 선언한 의미가 없어 보입니다.
제 생각에는 LoadingViewMessage는 Modifier로 제공하는 것보다 기본 파라미터를 옵셔널로 하는 것이 좋을거 같습니다! 해당 컴포넌트에서는 필수적인 기능이니까요
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.
앗..오타 스미마셍 ㅋㅎㅋㅎㅋㅎㅋㅎㅋㅎ
| VStack(spacing: 10) { | ||
| ProgressView() | ||
| .progressViewStyle(.circular) | ||
| .frame(width: size.size.width , height: size.size.height) |
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.
해당 부분은 controlSize() 수정자를 사용해서 아래와 같이 작성하는게 좋을거 같아요
| .frame(width: size.size.width , height: size.size.height) | |
| .controlSize(size == .large ? .large : .small) |
JEONG-J
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 유형
🛠️ 작업내용
커밋 히스토리
📋 추후 진행 상황
📌 리뷰 포인트
✅ Checklist
PR이 다음 요구 사항을 충족하는지 확인해주세요!!!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.