-
Notifications
You must be signed in to change notification settings - Fork 97
Conditional Visibility #2843
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: main
Are you sure you want to change the base?
Conditional Visibility #2843
Conversation
… into jzdesign/conditional-visibility-2
...ses/src/main/kotlin/com/revenuecat/purchases/paywalls/components/common/ComponentOverride.kt
Show resolved
Hide resolved
...c/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/ScreenConditionSnapshot.kt
Outdated
Show resolved
Hide resolved
📸 Snapshot Test570 unchanged
🛸 Powered by Emerge Tools |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2843 +/- ##
==========================================
- Coverage 78.28% 78.22% -0.07%
==========================================
Files 330 330
Lines 12753 12785 +32
Branches 1739 1747 +8
==========================================
+ Hits 9984 10001 +17
- Misses 2038 2046 +8
- Partials 731 738 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
#2843 renamed a key from `intro_offer` to `introductory_offer` which would break current paywalls that use `intro_offer` for "Text field for an introductory offer" section in the builder.
...ses/src/main/kotlin/com/revenuecat/purchases/paywalls/components/common/ComponentOverride.kt
Show resolved
Hide resolved
We noticed that phones get categorized as tablets in landscape, and tablets as desktops. I changed the calculation to use the width in portrait (the shortest dimension) instead. But we still need to discuss if this is what we want.
tonidero
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.
Still reviewing but posting what I have so far
|
|
||
| @Serializable | ||
| enum class EqualityOperatorType { | ||
| @SerialName("=") // WIP… Should we make this human language from the server like in and not_in? |
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 could be eq and not_eq to have the same style as the other operators indeed... but not a strong preference.
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.
Turns out, Khepri requires this. It's used a lot in other areas.
...ses/src/main/kotlin/com/revenuecat/purchases/paywalls/components/common/ComponentOverride.kt
Show resolved
Hide resolved
...c/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/ScreenConditionSnapshot.kt
Outdated
Show resolved
Hide resolved
| "selected" to { Condition.Selected.serializer() }, | ||
| "orientation" to { Condition.Orientation.serializer() }, | ||
| "screen_size" to { Condition.ScreenSize.serializer() }, | ||
| "selected_package" to { Condition.SelectedPackage.serializer() }, |
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.
What's the difference between selected and selected_package? And also between intro_offer and introductory_offer_available + multiple_intro_offers and multiple_intro_offers_available? Feels like a rename to support the new condition system, but basically this also means it would default to unsupported in older versions of the SDK... Not sure if that's worth it?
If I'm wrong and they are supposed to be different conditions, we might want to document the difference
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.
Good questions. Long story short, our foresight was only kind of right when we started this work many months ago. As development continued, other scenarios came up, and the use case for the keys that were already in the code could have been clearer but these are the meanings. I have a doc on this that I can throw into notion, or perhaps this is a safe enough one that could live in the repo… though it would be duplicated.
| Key | Purpose |
|---|---|
selected |
The component itself is selected, like a tab, a package, a switch, etc., |
selected_package |
The selected package matches the given array conditions |
intro_offer |
The selected package includes an intro offer you are eligible for |
introductory_offer_available |
any package in the paywall includes an introductory offer you are eligible for, selected or not |
multiple_intro_offers |
The selected package has multiple intro offers that you are eligible for |
multiple_intro_offers_available |
any package in the paywall includes multiple intro offers you are eligible for, selected or not |
app_version |
The consuming application version matches the given comparison |
Note
Edited, adding application version to allow for fine tuned control for various reasons like being able to add new conditions that are not supported by older SDKs and giving the user the ability to ensure the paywall renders appropriately for their use case.
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.
Ohh thanks for the explanation. That makes sense! Then, I would indeed add that as docs to the repo, I think it should be fine, and could help understand the difference between them.
...main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/stack/StackComponentState.kt
Show resolved
Hide resolved
...otlin/com/revenuecat/purchases/ui/revenuecatui/components/carousel/CarouselComponentState.kt
Outdated
Show resolved
Hide resolved
| var snapshot by mutableStateOf(ScreenConditionSnapshot()) | ||
| private set | ||
|
|
||
| fun updateLayoutSize(widthDp: Float, heightDp: Float) { |
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 I'm not in love with making all these methods available to callers, since they could potentially be called somewhere else, when they are only intended to be called by the rememberScreenConditionState and trackScreenCondition below... But I'm not sure what would be the best approach...
We could make rememberScreenConditionState a companion object function, and would allow us to make some of these private, but wouldn't work for trackScreenCondition since that's an extension function of Modifier... So I think it's ok to leave as is for now...
...main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/VideoComponentState.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/ScreenConditionState.kt
Outdated
Show resolved
Hide resolved
ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/data/PaywallState.kt
Outdated
Show resolved
Hide resolved
ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/data/PaywallState.kt
Outdated
Show resolved
Hide resolved
...atui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/PresentedPartial.kt
Show resolved
Hide resolved
tonidero
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.
Only that last VideoComponentState question, but it's looking great!
| selectedPackageProvider: () -> Package?, | ||
| selectedTabIndexProvider: () -> Int, | ||
| screenConditionProvider: () -> ScreenCondition, | ||
| introOfferAvailability: IntroOfferAvailability = IntroOfferAvailability(), |
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.
Same here. We probably should remove the default IntroOfferAvailability, probably a leftover? :)
|
|
||
| @JvmSynthetic | ||
| fun update( | ||
| windowSize: WindowWidthSizeClass? = null, |
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 I'm not sure this is right... in the other components states, the update method includes the screenCondition, so we can update it, but not for the VideoComponentState. Is there a reason for that difference? This also means that we will keep using the initialScreenCondition for the PresentedPartial even if the screen changes, so not sure if that's right... Maybe we're handling that in the view layer?
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.
Ah, this here isn't actually the issue but I'm glad it flagged things for you, because there was an issue that I fixed where build partial was called
Checklist
purchases-iosand hybridsMotivation
Users want a way to control when to show or hide certain elements of their paywall. Commonly this is based on screen size, but there are other reasons this may occur as well.
Description
Josh wrote the majority of the iOS PR months ago. I picked it up and added the selected package bit, then I had codex look at those changes and replicate them here in the android project.