Skip to content
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

Merged
merged 15 commits into from
Feb 17, 2025
Merged

Conversation

joshdholtz
Copy link
Member

@joshdholtz joshdholtz commented Jan 28, 2025

Motivation

Gotta carousel!

Description

Screen.Recording.2025-02-12.at.10.33.10.PM.mov

Copy link

emerge-tools bot commented Jan 28, 2025

1 build increased size

Name Version Download Change Install Change Approval
Paywalls
com.revenuecat.PaywallsTester
1.0 (1) 11.1 MB ⬆️ 211.3 kB (1.93%) 42.1 MB ⬆️ 793.4 kB (1.93%) N/A

Paywalls 1.0 (1)
com.revenuecat.PaywallsTester

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 793.4 kB (1.93%)
Total download size change: ⬆️ 211.3 kB (1.93%)

Largest size changes

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
View Treemap

Image of diff


🛸 Powered by Emerge Tools

Comment trigger: Size diff threshold of 100.00kB exceeded

@joshdholtz joshdholtz force-pushed the paywalls-v2/carousel-component branch from 14701f4 to fc5511d Compare January 28, 2025 12:16
public let margin: Padding
public let shape: Shape?
public let border: Border?
public let shadow: Shadow?
Copy link
Contributor

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?

Copy link
Contributor

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

tonidero added a commit to RevenueCat/purchases-android that referenced this pull request Jan 31, 2025
### 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
tonidero added a commit to RevenueCat/purchases-android that referenced this pull request Feb 3, 2025
### 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
@tonidero
Copy link
Contributor

tonidero commented Feb 3, 2025

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!

@joshdholtz joshdholtz force-pushed the paywalls-v2/carousel-component branch from fc5511d to e9d1687 Compare February 13, 2025 02:32
@joshdholtz joshdholtz changed the base branch from main to paywalls-v2/visible-property February 13, 2025 02:32
@joshdholtz joshdholtz force-pushed the paywalls-v2/visible-property branch 2 times, most recently from f8286e8 to 5bc8084 Compare February 13, 2025 19:48
@joshdholtz joshdholtz force-pushed the paywalls-v2/carousel-component branch from a17ecb0 to f734a6f Compare February 14, 2025 01:35
Copy link
Contributor

@tonidero tonidero left a 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
Copy link
Contributor

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.

Comment on lines 41 to 42
public let padding: Padding
public let margin: Padding
Copy link
Contributor

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,
Copy link
Contributor

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?

Copy link
Member

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

Copy link
Member Author

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 😅

Copy link
Member

@ajpallares ajpallares left a 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 {
Copy link
Member

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,
Copy link
Member

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) {
Copy link
Member

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
Suggested change
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 }

Comment on lines 409 to 411
if self.originalCount <= 1 {
EmptyView()
} else {
Copy link
Member

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()

Suggested change
if self.originalCount <= 1 {
EmptyView()
} else {
if self.originalCount > 1 {

.animation(.easeInOut, value: self.localCurrentIndex)
}
}
.padding(self.pageControl.padding)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense here to ignore the horizontal padding? I say this to avoid these kinds of clipping of the cards on the edges. But I don't know if ignoring these paddings could have other undesired consequences
image

Copy link
Contributor

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.

Base automatically changed from paywalls-v2/visible-property to main February 14, 2025 12:56
@joshdholtz joshdholtz force-pushed the paywalls-v2/carousel-component branch from f734a6f to 4980a17 Compare February 14, 2025 19:15
@joshdholtz joshdholtz changed the title WIP: Carousel compnent [Paywalls V2] Carousel component Feb 14, 2025
@joshdholtz joshdholtz marked this pull request as ready for review February 14, 2025 19:35
Copy link
Contributor

@tonidero tonidero left a 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)
Copy link
Contributor

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)

Copy link
Member

@ajpallares ajpallares left a 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 🤯

@tonidero tonidero merged commit b4c81d1 into main Feb 17, 2025
10 checks passed
@tonidero tonidero deleted the paywalls-v2/carousel-component branch February 17, 2025 08:34
This was referenced Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants