Creates default PaywallData by vegaro · Pull Request #1261 · RevenueCat/purchases-android

vegaro · 2023-09-19T14:38:17Z

Creates a default PaywallData in case data doesn't pass validation

vegaro · 2023-09-20T09:55:27Z

@tonidero do you mind giving this a quick look see if it makes sense so far? Specially the Colors related stuff

tonidero

I think the color stuff makes sense 👍

NachoSoto · 2023-09-19T15:58:03Z

+            blurredBackgroundImage = true,
+            displayRestorePurchases = true,
+        ),
+        localization = mapOf(locale to PaywallData.defaultLocalization), // TODO-PAYWALLS: test this


This makes sense

Ok, I wasn't sure haha

It's basically taking the system one 🤷‍♂️

So defaultLocalization is not currently localized right? I was wondering whether to hardcode the Locale.ENGLISH for now until that was localized. But if we are doing that soon, it should be ok to leave this.

I think that makes sense! to use Locale.ENGLISH

…ate-default-paywalldata # Conflicts: # ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/templates/Template2.kt

codecov · 2023-09-21T16:45:34Z

Codecov Report

Patch coverage: 60.00% and project coverage change: -0.01% ⚠️

Comparison is base (a4ecd77) 85.47% compared to head (0f9e68a) 85.46%.
Report is 1 commits behind head on paywalls.

Additional details and impacted files

@@             Coverage Diff              @@
##           paywalls    #1261      +/-   ##
============================================
- Coverage     85.47%   85.46%   -0.01%     
============================================
  Files           190      190              
  Lines          6380     6385       +5     
  Branches        929      930       +1     
============================================
+ Hits           5453     5457       +4     
  Misses          568      568              
- Partials        359      360       +1

Files Changed	Coverage Δ
.../com/revenuecat/purchases/paywalls/PaywallColor.kt	`68.42% <60.00%> (+4.13%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

NachoSoto · 2023-09-21T17:31:56Z

        },
    )
+
+    constructor(@ColorInt colorInt: Int) : this(


Can we add tests for this?

Added a test for this constructor. I plan on adding a bunch of tests after this gets merged

NachoSoto

Awesome 👏🏻 just a few comments.

NachoSoto · 2023-09-21T17:34:10Z

+@ReadOnlyComposable
+private fun PaywallData.Companion.createDefaultForIdentifiers(packageIdentifiers: List<String>): PaywallData {
+    val context = LocalContext.current
+    val locale = LocaleListCompat.getDefault()[0].toString()


This is duplicated in PaywallData.localizedConfiguration. Maybe we can add a property there to make sure we use the same default?

I did this #1261 (comment) so this is not needed anymore

NachoSoto · 2023-09-21T17:38:04Z

+        config = PaywallData.Configuration(
+            packages = packageIdentifiers,
+            images = PaywallData.Configuration.Images(
+                background = PaywallData.defaultBackgroundImage,


Nice, does this work now?

It doesn't. I created a https://linear.app/revenuecat/issue/PWL-274/support-default-background-image to make sure this works, for now it is just a placeholder. We need to do some refactor and change URL with URI for loading images from anywhere

tonidero · 2023-09-22T08:21:04Z

+            blurredBackgroundImage = true,
+            displayRestorePurchases = true,
+        ),
+        localization = mapOf(locale to PaywallData.defaultLocalization), // TODO-PAYWALLS: test this


So defaultLocalization is not currently localized right? I was wondering whether to hardcode the Locale.ENGLISH for now until that was localized. But if we are doing that soon, it should be ok to leave this.

tonidero · 2023-09-22T08:27:27Z

+@Composable
+@ReadOnlyComposable
+private fun PaywallData.Companion.createDefaultForIdentifiers(packageIdentifiers: List<String>): PaywallData {
+    val context = LocalContext.current


Note that the fact that we have this in the same place that we calculate the PaywallData means that, whenever there is a configuration change, this and anything depending on this function will get recompositioned.

Additionally, I'm not totally sure how we're planning to use this, since the paywall data is currently obtained/stored by the view model, which can't access composable functions.

Made the change in 6ce4758 and these are not composable anymore

tonidero

Left some comments. I'm wondering if the background image loads well... Other than that, nothing blocking

tonidero · 2023-09-22T14:51:23Z

    }

+    @Test
+    fun `paywall color can be created from a ColorInt`() {


Hmm I was thinking whether we should move this test to a different file... But I think it's ok for now since it's indeed kinda related.

Yeah, let's create a new file that will only have this test for now. It was me being lazy

tonidero · 2023-09-22T14:59:00Z

+    get() = -1
+
+private val PaywallData.Companion.defaultBackgroundImage: String
+    get() = "R.drawable.default_background"


So then, it will work with RemoteImage when using a local resource for the background? Checking just to make sure.

I will address the background image stuff in a future PR because it requires some refactoring and changing URL to URIs

https://linear.app/revenuecat/issue/PWL-274/support-default-background-image

vegaro · 2023-09-22T15:00:12Z

+    )
+
+private fun PaywallData.Companion.defaultTemplateBaseURL(packageName: String): URL =
+    URL("android.resource://$packageName/")


I tested this and it doesn't work, so I am just going to revert it to use a placeholder, same way we do in iOS. It requires less refactoring

Oh looks like you never reverted this. It's making the app crash right now.

NachoSoto · 2023-09-22T15:28:06Z

+    )
+
+private fun PaywallData.Companion.defaultTemplateBaseURL(packageName: String): URL =
+    URL("android.resource://$packageName/")


NachoSoto · 2023-09-22T15:28:26Z

+                .widthIn(max = Template2UIConstants.maxIconWidth)
+                .clip(RoundedCornerShape(Template2UIConstants.iconCornerRadius))
+            if (uri.toString().contains(PaywallData.defaultAppIconPlaceholder)) {
+                AppIcon(


NachoSoto · 2023-09-22T15:29:15Z

+        accent2 = currentColors.primary.asPaywallColor(),
+    )
+}
+


For a future PR: we could add a preview in this file that shows Template2() with this default data :)

https://linear.app/revenuecat/issue/PWL-277/create-preview-with-default-data

creates default PaywallData

d0e237b

vegaro added the pr:feat A new feature label Sep 19, 2023

RevenueCat deleted a comment from RevenueCat-Danger-Bot Sep 19, 2023

add colors

4ef37df

vegaro commented Sep 20, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/extensions/PaywallDataExtensions.kt Outdated

tonidero reviewed Sep 20, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/extensions/PaywallDataExtensions.kt Outdated

NachoSoto reviewed Sep 20, 2023

View reviewed changes

vegaro added 2 commits September 21, 2023 15:09

change visibility of defaults

5ffac96

add docstring

a898026

NachoSoto reviewed Sep 21, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/extensions/PaywallDataExtensions.kt Outdated

vegaro added 2 commits September 21, 2023 18:28

default image

798c5c8

Merge remote-tracking branch 'origin/paywalls' into cesar/pwl-192-cre…

92f52bc

…ate-default-paywalldata # Conflicts: # ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/templates/Template2.kt

NachoSoto mentioned this pull request Sep 21, 2023

Paywalls: created LoadingPaywallView #1282

Merged

use template id

acdf402

vegaro marked this pull request as ready for review September 21, 2023 16:56

vegaro requested review from a team, NachoSoto and tonidero September 21, 2023 16:56

vegaro changed the title ~~[WIP] Creates default PaywallData~~ Creates default PaywallData Sep 21, 2023

NachoSoto reviewed Sep 21, 2023

View reviewed changes

tonidero reviewed Sep 22, 2023

View reviewed changes

vegaro added 5 commits September 22, 2023 12:52

Merge branch 'paywalls' into cesar/pwl-192-create-default-paywalldata

8539f87

use PaywallTemplate

fb521ec

add test for PaywallColor constructor with ColorInt

bf613b9

default to Locale.ENGLISH

a19a1bf

fix icon

f9bd6d9

vegaro added 5 commits September 22, 2023 16:30

fix icon

dd1e744

rename to getThemeColors

2d2440b

remove composable annotations

6ce4758

rename to default_background

081a03d

remove TODO

f74fc43

vegaro requested review from NachoSoto and tonidero September 22, 2023 14:48

tonidero approved these changes Sep 22, 2023

View reviewed changes

vegaro commented Sep 22, 2023

View reviewed changes

vegaro added 4 commits September 22, 2023 17:06

use Locale.US

673edfa

don't remember depending on context

affa1d7

extract test

c2b48c1

added todos for background image

0f9e68a

NachoSoto approved these changes Sep 22, 2023

View reviewed changes

NachoSoto reviewed Sep 22, 2023

View reviewed changes

vegaro merged commit af717d8 into paywalls Sep 22, 2023

vegaro deleted the cesar/pwl-192-create-default-paywalldata branch September 22, 2023 15:59

This was referenced Oct 16, 2023

[DO NOT MERGE] Release/7.1.0-beta.1 #1355

Closed

[DO NOT MERGE] Release/7.1.0-beta.2 #1373

Closed

Conversation

vegaro commented Sep 19, 2023

Uh oh!

vegaro commented Sep 20, 2023

Uh oh!

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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

codecov Bot commented Sep 21, 2023 •

edited

Loading