-
Notifications
You must be signed in to change notification settings - Fork 338
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
[Paywalls V2] Carousel component #4722
Conversation
1 build increased size
Paywalls 1.0 (1)
|
Item | Install Size Change |
---|---|
DYLD.String Table | ⬆️ 261.4 kB |
📝 RevenueCatUI.CarouselComponentViewModel.CarouselComponentViewMode... | ⬆️ 23.4 kB |
Code Signature | ⬆️ 20.9 kB |
DYLD.Exports | ⬆️ 12.4 kB |
📝 RevenueCat.PaywallComponent.PartialCarouselComponent.PartialCarou... | ⬆️ 9.5 kB |
🛸 Powered by Emerge Tools
Comment trigger: Size diff threshold of 100.00kB exceeded
14701f4
to
fc5511d
Compare
public let margin: Padding | ||
public let shape: Shape? | ||
public let border: Border? | ||
public let shadow: Shadow? |
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.
Should we also add a background color property? Also do we need to add the page indicator settings here?
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.
Another property that we might want is the initialPage to use
### Description This adds the carousel component structure and data flow. It does not implement the view, which will come in a follow up PR. First part of Android equivalent of: RevenueCat/purchases-ios#4722 - [ ] Finalize schema
### Description This adds the `CarouselComponentView` that actually displays the carousel Second part of Android equivalent of RevenueCat/purchases-ios#4722 /~https://github.com/user-attachments/assets/9649e370-d207-4b81-9119-7c60dc9cff13
I merged a version of this in Android. The schema I used is in this PR: RevenueCat/purchases-android#2092. Though we can perform any modifications if needed of course! |
fc5511d
to
e9d1687
Compare
f8286e8
to
5bc8084
Compare
a17ecb0
to
f734a6f
Compare
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.
LGTM! Lmk when it's ready for a final review and I can approve it!
|
||
let spacing: CGFloat | ||
let active: DisplayablePageControlIndicator | ||
let `default`: DisplayablePageControlIndicator |
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.
Lmk if you would prefer changing the name of this property to avoid the backlashes. Shouldn't be a problem.
public let padding: Padding | ||
public let margin: Padding |
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.
Hmm in Android I made these optional (defaulting to 0 padding/margin), not sure if it matters much though.
initialIndex: Int, | ||
loop: Bool = false, | ||
spacing: CGFloat = 16, | ||
cardWidth: CGFloat = 300, |
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.
Hmm seems weird to have these defaults? 🤔 Seems these should be in the previews if needed?
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.
I was going to comment the same. If we're passing values anyways, we should avoid the defaults. I've been burned in the past by unnecessary default values
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.
Ooooops... this was here from my POC before I had an API figured out 😅
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.
Some comments for now
) | ||
) { style in | ||
Group { | ||
if style.visible { |
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.
Remember, to remove this group you can make the apply
parameter of the view model a @ViewBuilder
initialIndex: Int, | ||
loop: Bool = false, | ||
spacing: CGFloat = 16, | ||
cardWidth: CGFloat = 300, |
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.
I was going to comment the same. If we're passing values anyways, we should avoid the defaults. I've been burned in the past by unnecessary default values
pauseAutoPlay(for: 10) | ||
} | ||
|
||
private func pauseAutoPlay(for seconds: TimeInterval) { |
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.
Perhaps this method can do an early return if autoplay is off (to avoid unnecessary operations). Something like this
private func pauseAutoPlay(for seconds: TimeInterval) { | |
private var autoPlayEnabled: Bool { | |
return self.msTimePerSlide != nil && self.msTransitionTime != nil | |
} | |
private func pauseAutoPlay(for seconds: TimeInterval) { | |
guard self.autoPlayEnabled else { return } | |
if self.originalCount <= 1 { | ||
EmptyView() | ||
} else { |
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.
Perhaps we don't need the EmptyView()
if self.originalCount <= 1 { | |
EmptyView() | |
} else { | |
if self.originalCount > 1 { |
.animation(.easeInOut, value: self.localCurrentIndex) | ||
} | ||
} | ||
.padding(self.pageControl.padding) |
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.
Hmm it's a good point... But for now, I think we should just limit setting it in the dashboard.
f734a6f
to
4980a17
Compare
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.
LGTM!
} | ||
|
||
// Pause auto-play for 10 seconds | ||
pauseAutoPlay(for: 10) |
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.
In android, I currently disable manual dragging if autoplay is enabled... I guess we want to allow this there? not sure if we might want it to be a setting though... (Not a blocker for this 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.
Such an incredible PR 🤯
Motivation
Gotta carousel!
Description
Screen.Recording.2025-02-12.at.10.33.10.PM.mov