[Paywalls V2] Carousel component by joshdholtz · Pull Request #4722 · RevenueCat/purchases-ios

joshdholtz · 2025-01-28T12:07:59Z

Motivation

Gotta carousel!

Description

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

emerge-tools · 2025-01-28T12:14:52Z

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

🛸 Powered by Emerge Tools

Comment trigger: Size diff threshold of 100.00kB exceeded

tonidero · 2025-01-28T16:37:41Z

+        public let margin: Padding
+        public let shape: Shape?
+        public let border: Border?
+        public let shadow: Shadow?


Should we also add a background color property? Also do we need to add the page indicator settings here?

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

tonidero · 2025-02-03T13:20:34Z

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!

tonidero

LGTM! Lmk when it's ready for a final review and I can approve it!

tonidero · 2025-02-14T07:54:24Z

+
+    let spacing: CGFloat
+    let active: DisplayablePageControlIndicator
+    let `default`: DisplayablePageControlIndicator


Lmk if you would prefer changing the name of this property to avoid the backlashes. Shouldn't be a problem.

tonidero · 2025-02-14T07:55:40Z

+            public let padding: Padding
+            public let margin: Padding


Hmm in Android I made these optional (defaulting to 0 padding/margin), not sure if it matters much though.

tonidero · 2025-02-14T08:03:09Z

+        initialIndex: Int,
+        loop: Bool = false,
+        spacing: CGFloat = 16,
+        cardWidth: CGFloat = 300,


Hmm seems weird to have these defaults? 🤔 Seems these should be in the previews if needed?

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

Ooooops... this was here from my POC before I had an API figured out 😅

ajpallares

Some comments for now

ajpallares · 2025-02-14T08:58:57Z

+            )
+        ) { style in
+            Group {
+                if style.visible {


Remember, to remove this group you can make the apply parameter of the view model a @ViewBuilder

ajpallares · 2025-02-14T09:03:36Z

+        initialIndex: Int,
+        loop: Bool = false,
+        spacing: CGFloat = 16,
+        cardWidth: CGFloat = 300,


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

ajpallares · 2025-02-14T09:08:32Z

+        pauseAutoPlay(for: 10)
+    }
+
+    private func pauseAutoPlay(for seconds: TimeInterval) {


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 }

ajpallares · 2025-02-14T09:11:05Z

+        if self.originalCount <= 1 {
+            EmptyView()
+        } else {


Perhaps we don't need the EmptyView()

Suggested change

if self.originalCount <= 1 {

EmptyView()

} else {

if self.originalCount > 1 {

ajpallares · 2025-02-14T09:18:16Z

+                        .animation(.easeInOut, value: self.localCurrentIndex)
+                }
+            }
+            .padding(self.pageControl.padding)


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

Hmm it's a good point... But for now, I think we should just limit setting it in the dashboard.

…it fit

tonidero

LGTM!

tonidero · 2025-02-17T07:54:03Z

+        }
+
+        // Pause auto-play for 10 seconds
+        pauseAutoPlay(for: 10)


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)

ajpallares

Such an incredible PR 🤯

joshdholtz added pr:feat A new feature pr:RevenueCatUI feat:PaywallV2 labels Jan 28, 2025

joshdholtz force-pushed the paywalls-v2/carousel-component branch from 14701f4 to fc5511d Compare January 28, 2025 12:16

tonidero reviewed Jan 28, 2025

View reviewed changes

This was referenced Jan 31, 2025

[Paywalls V2] Add CarouselComponent infrastructure RevenueCat/purchases-android#2092

Merged

[Paywalls V2] Add CarouselComponentView RevenueCat/purchases-android#2095

Merged

joshdholtz force-pushed the paywalls-v2/carousel-component branch from fc5511d to e9d1687 Compare February 13, 2025 02:32

joshdholtz changed the base branch from main to paywalls-v2/visible-property February 13, 2025 02:32

joshdholtz force-pushed the paywalls-v2/visible-property branch 2 times, most recently from f8286e8 to 5bc8084 Compare February 13, 2025 19:48

joshdholtz force-pushed the paywalls-v2/carousel-component branch from a17ecb0 to f734a6f Compare February 14, 2025 01:35

tonidero reviewed Feb 14, 2025

View reviewed changes

ajpallares reviewed Feb 14, 2025

View reviewed changes

Base automatically changed from paywalls-v2/visible-property to main February 14, 2025 12:56

joshdholtz added 11 commits February 14, 2025 13:15

This mostly works

cad26eb

Some cleanup and todos

dc7ab89

Fixed

b267c6f

BROKEN but need to rebase

2d134d4

The rework happened

04be0de

This is feeling pretty good

62cc41b

This works

7e72dec

Fix lint

01cc41d

Oops

0b8be8b

Or this

e914ca2

Removed size and implemented top and bottom pager

e40d933

joshdholtz added 2 commits February 14, 2025 13:15

Added a todo and compile error

b4af3bc

Made page control optional and added optional size that defaults to f…

4980a17

…it fit

joshdholtz force-pushed the paywalls-v2/carousel-component branch from f734a6f to 4980a17 Compare February 14, 2025 19:15

Fixes from PR review

ebedab6

joshdholtz changed the title ~~WIP: Carousel compnent~~ [Paywalls V2] Carousel component Feb 14, 2025

joshdholtz requested review from ajpallares and tonidero February 14, 2025 19:26

joshdholtz marked this pull request as ready for review February 14, 2025 19:35

Fix podlint

4e53c52

tonidero approved these changes Feb 17, 2025

View reviewed changes

ajpallares approved these changes Feb 17, 2025

View reviewed changes

tonidero merged commit b4c81d1 into main Feb 17, 2025

tonidero deleted the paywalls-v2/carousel-component branch February 17, 2025 08:34

This was referenced Feb 19, 2025

[AUTOMATIC] Release/5.18.0 #4802

Closed

[AUTOMATIC] Release/5.18.0 #4815

Merged

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

Uh oh!

Conversation

joshdholtz commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description

Uh oh!

emerge-tools Bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 build increased size

Paywalls 1.0 (1)com.revenuecat.PaywallsTester

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented Feb 3, 2025

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajpallares left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajpallares left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joshdholtz commented Jan 28, 2025 •

edited

Loading

emerge-tools Bot commented Jan 28, 2025 •

edited

Loading

Paywalls 1.0 (1)
com.revenuecat.PaywallsTester