-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add RevenueCatUI module and initial API #1213
Conversation
implementation libs.androidx.lifecycle.viewmodel | ||
implementation libs.androidx.lifecycle.viewmodel.compose | ||
debugImplementation libs.compose.ui.tooling | ||
debugImplementation libs.androidx.test.compose.manifest |
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.
Most of this is copied over from the debugview
module
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## paywalls #1213 +/- ##
===========================================
Coverage ? 85.25%
===========================================
Files ? 186
Lines ? 6319
Branches ? 911
===========================================
Hits ? 5387
Misses ? 586
Partials ? 346 ☔ View full report in Codecov by Sentry. |
Nice!! |
I wonder... Since it's a different module, the chances of conflicts are mostly minimized and we have disabled shipping it for now. We might need to once we add changes to the main SDK for the paywall data, but even then, I think it's ok to try to merge into Edit: Talked on Slack, we will keep it in a branch for now |
*/ | ||
@Suppress("unused") | ||
@Composable | ||
fun PaywallFooter(options: PaywallFooterOptions = PaywallFooterOptions.Builder().build()) { |
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 renamed the PaywallBottomSheet
to PaywallFooter
, since now this won't include the full screen option. It will still be displayed as a bottom sheet, but it made more sense to keep naming the same as in iOS in this 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.
Additionally, I moved the parameters to use the builder pattern. My main reasons is that it will be much easier to add/deprecate options in the future. Since this is a function, keeping deprecated parameters can potentially grow and adding new parameters would need to keep the order to be usable.
It will indeed make configuring this sligthly harder... But I think it's acceptable. Lmk if you think otherwise.
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 remove this for now until we work on this mode?
In general I don't think this should be its own type
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 makes sense! Will remove the footer 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.
Maybe PaywallFooterOptions
too?
I made some changes to the API from our latest conversations. Please lmk what you think @RevenueCat/coresdk ! I still haven't added an API to present the paywall full screen as a dialog, since I want to do a bit more research on that. |
7b922b8
to
909596f
Compare
### Description This PR creates a new module `:ui:revenuecatui` as a starting point for paywalls. Additionally, it adds some initial code for the public API.
Description
This PR creates a new module
:ui:revenuecatui
as a starting point for paywalls. Additionally, it adds some initial code for the public API.