Skip to content
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 PaywallData and its classes #1219

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented Sep 4, 2023

Just adds the PaywallData class and its classes. Followup PRs will parse them and add them to the Offerings class

@vegaro vegaro added the feat A new feature label Sep 4, 2023
@vegaro vegaro requested a review from a team September 4, 2023 15:24
@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.68% ⚠️

Comparison is base (5dae4fd) 85.93% compared to head (b34014a) 85.25%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1219      +/-   ##
==========================================
- Coverage   85.93%   85.25%   -0.68%     
==========================================
  Files         184      186       +2     
  Lines        6269     6319      +50     
  Branches      911      911              
==========================================
  Hits         5387     5387              
- Misses        536      586      +50     
  Partials      346      346              
Files Changed Coverage Δ
.../com/revenuecat/purchases/paywalls/PaywallColor.kt 0.00% <0.00%> (ø)
...n/com/revenuecat/purchases/paywalls/PaywallData.kt 0.00% <0.00%> (ø)

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

* Creates a color from a Hex string: `#RRGGBB` or `#RRGGBBAA`.
*/
@RequiresApi(Build.VERSION_CODES.O)
constructor(stringRepresentation: String) : this(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valueOf is only available from Android O +

/**
* Image displayed as an app icon in a template.
*/
val icon: String? = null,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= null is so we have the empty constructor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm why do we need an empty constructor? If there are any templates where each of these images can be null, then this makes sense though.

Copy link
Contributor Author

@vegaro vegaro Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/**
* Secondary accent color.
*/
val accent2: PaywallColor? = null,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, these are = null so there's a constructor without them. I saw the iOS code did this and thought it was convenient

import androidx.annotation.RequiresApi

/**
* Represents a color to be used by `RevenueCatUI`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left this as RevenueCatUI until we define the name of the library

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be pointing to the paywalls branch I think, so we don't expose these before we have to. Nothing major, other than that 👍

/**
* The base remote URL where assets for this paywall are stored.
*/
val assetBaseURL: URL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I wonder if we need to expose this, or if we could just build the URL for each asset... I guess I'm ok following with this, if it's easier, or we can change it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iOS SDK also has this as a public property, and also builds the whole URL for each asset in a TemplateViewConfiguration that I believe is not public. I am not sure the reasoning why assetBaseURL is public though. cc: @NachoSoto

/**
* Image displayed as an app icon in a template.
*/
val icon: String? = null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm why do we need an empty constructor? If there are any templates where each of these images can be null, then this makes sense though.

@NachoSoto
Copy link
Contributor

Nice!! I'll look at this more in detail tomorrow.
Should the PR target the paywalls branch?

@vegaro vegaro changed the base branch from main to paywalls September 5, 2023 08:25
@vegaro
Copy link
Contributor Author

vegaro commented Sep 5, 2023

Updated to point against the paywalls branch

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can answer those questions later, for now, this looks good

@vegaro vegaro merged commit c170c6d into paywalls Sep 5, 2023
7 checks passed
@vegaro vegaro deleted the cesar/pwl-148-sdk-parse-offerings-response-paywall-data-and-add branch September 5, 2023 12:18
tonidero pushed a commit that referenced this pull request Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants