-
Notifications
You must be signed in to change notification settings - Fork 0
Fix/#168 2차 QA 수정 #171
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?
Fix/#168 2차 QA 수정 #171
Conversation
📝 Walkthrough내용 요약2차 QA 수정을 위해 온보딩 화면 레이아웃 및 스타일을 조정하고, 홈 화면 이미지 위치를 재배치하며, 색상 자산을 업데이트하고, SVG 이미지 자산을 PNG 해상도 변수로 교체했습니다. 변경 사항
예상 코드 리뷰 난이도🎯 3 (Moderate) | ⏱️ ~25 분 관련 가능성 있는 PR
제안 검토자
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Cherrish-iOS/Cherrish-iOS.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
1-15: lottie-ios 의존성 제거가 불완전함 - 빌드 실패 예상Package.resolved에서 lottie-ios가 제거되었지만, 코드베이스는 여전히 Lottie를 적극적으로 사용 중입니다:
SplashView.swift와ChallengeLoadingView.swift에서LottieView컴포넌트 사용Lottie+.swift에LottieView구현 (UIViewRepresentable)project.pbxproj에 Lottie 프레임워크 참조 여전히 존재Resources/Lottie/splash.json애니메이션 파일 존재이 상태로는
import Lottie때문에 빌드가 실패합니다. Lottie 의존성을 Package.resolved에 복구하거나, 모든 Lottie 관련 코드를 제거해야 합니다.Cherrish-iOS/Cherrish-iOS/Resources/Assets.xcassets/home_icon/home_chellenge_bar0.imageset/Contents.json (1)
4-14: 1x/2x/3x 리소스 파일명이 불일치합니다.
1x는home_chellenge_bar0.svg인데 2x/3x는challenge_gaugebar_1@2x/@3x로 서로 다른 아트워크가 매핑됩니다. 1x 디바이스에서 다른 이미지가 노출될 수 있으니, 의도된 게 아니라면 동일한 파일명으로 통일해 주세요.🔧 예시 수정안 (1x도 동일한 PNG로 정렬)
- "filename" : "home_chellenge_bar0.svg", + "filename" : "challenge_gaugebar_1.png",
🤖 Fix all issues with AI agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Onboarding/OnboardingContainerView.swift`:
- Around line 45-50: The button-containing view that currently uses
.opacity(currentPage == 1 ? 1 : 0) can still receive taps when transparent;
update that view (the view with the .opacity and .animation modifiers in
OnboardingContainerView) to block hit testing when not visible by adding
.allowsHitTesting(currentPage == 1) (or .disabled(currentPage != 1)) alongside
the existing modifiers so taps are only accepted on the active page.
| .padding(.top, 21.adjustedH) | ||
| .padding(.horizontal, 25.adjustedW) | ||
| .padding(.bottom, 57.adjustedH) | ||
| .opacity(currentPage == 1 ? 1 : 0) | ||
| .animation(.easeInOut(duration: 0.3), value: currentPage) | ||
|
|
||
| Spacer() |
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.
투명 상태에서도 버튼이 탭 가능할 수 있습니다.
opacity만 적용하면 page 0에서도 탭 이벤트가 발생할 수 있어 의도치 않은 이동이 가능합니다. 페이지에 따라 히트테스트를 막아주세요.
✅ 권장 수정
.opacity(currentPage == 1 ? 1 : 0)
+ .allowsHitTesting(currentPage == 1)🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Onboarding/OnboardingContainerView.swift`
around lines 45 - 50, The button-containing view that currently uses
.opacity(currentPage == 1 ? 1 : 0) can still receive taps when transparent;
update that view (the view with the .opacity and .animation modifiers in
OnboardingContainerView) to block hit testing when not visible by adding
.allowsHitTesting(currentPage == 1) (or .disabled(currentPage != 1)) alongside
the existing modifiers so taps are only accepted on the active page.
y-eonee
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.
어프르브
🔗 연결된 이슈