-
Notifications
You must be signed in to change notification settings - Fork 54
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: Add API to display paywall as a composable dialog #1297
Paywalls: Add API to display paywall as a composable dialog #1297
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## paywalls #1297 +/- ##
=========================================
Coverage 85.46% 85.46%
=========================================
Files 190 190
Lines 6385 6385
Branches 930 930
=========================================
Hits 5457 5457
Misses 568 568
Partials 360 360 ☔ View full report in Codecov by Sentry. |
shouldDisplayDialog = try { | ||
// TODO-PAYWALLS: This won't receive updates in case the customer info changes and starts/stops | ||
// being eligible to display the paywall dialog. We would need to support multiple customer info | ||
// listeners to refresh this. |
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.
As mentioned, we would need to support multiple customer info listeners if we wanted to display this paywall after the fact... But I think it might be ok, at least for V1?
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.
Yeah that's how it works in iOS. We just want to do one initial check. I'd remove the TODO, since we wouldn't want to get notified of customer info changes and hide the paywall.
// TODO-PAYWALLS: This won't receive updates in case the customer info changes and starts/stops | ||
// being eligible to display the paywall dialog. We would need to support multiple customer info | ||
// listeners to refresh this. | ||
val customerInfo = Purchases.sharedInstance.awaitCustomerInfo() |
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 didn't fully like this here so I was thinking about moving it to the view model. But then, I didn't want to have to create the view model at this point (since we might not even display the dialog)... I thought it was ok for now to keep this. Lmk if you would prefer me to move it.
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.
This is fine. That's exactly how PresentingPaywallModifier
works.
|
||
@Composable | ||
@ReadOnlyComposable | ||
private fun shouldUsePlatformDefaultWidth(): Boolean { |
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.
This basically will display a full screen dialog for phones and a normal dialog for tablets and foldables. I think it looks ok for now but we can revisit later and polish this logic.
@@ -12,5 +12,5 @@ interface PaywallViewListener { | |||
fun onRestoreStarted() {} | |||
fun onRestoreCompleted(customerInfo: CustomerInfo) {} | |||
fun onRestoreError(error: PurchasesError) {} | |||
fun onDismissed() {} | |||
fun onCloseButtonPressed() {} |
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'm changing this listener method, since the onDismissed
only made sense when displaying as a dialog and the dialog options will already have a separate callback for that.
I'm making showing the close button optional (currently not implemented) and having a callback here for when it's tapped.
Template1MainContent(state) | ||
Column(modifier = Modifier.weight(1f)) { | ||
Template1MainContent(state) | ||
} |
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.
This fixes an issue that happened in some devices, specially in landscape where the purchase button and footer weren't visible. By adding a weight to the main content, it has less priority than the button/footer so those will always show.
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.
Would adding the weight to the outer colum work too?
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.
No, if we apply the weight to the outer column, that means that it won't apply to the inner views. Weights work with siblings by allowing you to set a relative weight between them to use the available space. In case one or more of the siblings don't have a weight attribute, those will have priority over those that do have it, which is what I'm doing here (give priority to the button + footer over the main content to display on the screen)
Template2MainContent(state, viewModel) | ||
Spacer(modifier = Modifier.weight(1f)) | ||
Box( | ||
modifier = Modifier.weight(1f).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.
Similar to the above, but for template 2.
@@ -55,6 +55,9 @@ private fun getPaywallViewModel( | |||
): PaywallViewModel { | |||
val applicationContext = LocalContext.current.applicationContext | |||
return viewModel<PaywallViewModelImpl>( | |||
// We need to pass the key in order to create different view models for different offerings when | |||
// trying to load different paywalls for the same view model store owner. | |||
key = offering?.identifier, |
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.
This has an important drawback which is that, if a user displays multiple different offerings as dialogs, the viewmodels for all of them will remain in memory... I'm not sure if this is the best way to fix this, but it should work for now.
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 think that makes sense. It's also probably extremely uncommon.
3736875
to
84fb593
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.
Looks great!
@@ -55,6 +55,9 @@ private fun getPaywallViewModel( | |||
): PaywallViewModel { | |||
val applicationContext = LocalContext.current.applicationContext | |||
return viewModel<PaywallViewModelImpl>( | |||
// We need to pass the key in order to create different view models for different offerings when | |||
// trying to load different paywalls for the same view model store owner. | |||
key = offering?.identifier, |
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 think that makes sense. It's also probably extremely uncommon.
shouldDisplayDialog = try { | ||
// TODO-PAYWALLS: This won't receive updates in case the customer info changes and starts/stops | ||
// being eligible to display the paywall dialog. We would need to support multiple customer info | ||
// listeners to refresh this. |
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.
Yeah that's how it works in iOS. We just want to do one initial check. I'd remove the TODO, since we wouldn't want to get notified of customer info changes and hide the paywall.
// TODO-PAYWALLS: This won't receive updates in case the customer info changes and starts/stops | ||
// being eligible to display the paywall dialog. We would need to support multiple customer info | ||
// listeners to refresh this. | ||
val customerInfo = Purchases.sharedInstance.awaitCustomerInfo() |
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.
This is fine. That's exactly how PresentingPaywallModifier
works.
} | ||
if (shouldDisplayDialog) { | ||
Logger.d("Displaying paywall dialog according to display logic") | ||
} else Logger.d("Not displaying paywall dialog according to display logic") |
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.
Nit: weird that you have {} for the if
but not the else
.
import kotlinx.coroutines.launch | ||
|
||
@Composable | ||
fun PaywallDialog( |
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.
Can you add a docstring to this?
Template1MainContent(state) | ||
Column(modifier = Modifier.weight(1f)) { | ||
Template1MainContent(state) | ||
} |
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.
Would adding the weight to the outer colum work too?
### Description This adds a new composable `PaywallDialog` that can be used to display the paywall as a dialog. It also modifies paywall tester to provide options on how we want to display the paywall from the offerings tab.
Description
This adds a new composable
PaywallDialog
that can be used to display the paywall as a dialog. It also modifies paywall tester to provide options on how we want to display the paywall from the offerings tab.