Skip to content

Paywalls: Add PaywallView#1510

Merged
vegaro merged 5 commits into
add-paywall-footer-viewfrom
add-paywall-view
Dec 5, 2023
Merged

Paywalls: Add PaywallView#1510
vegaro merged 5 commits into
add-paywall-footer-viewfrom
add-paywall-view

Conversation

@tonidero

@tonidero tonidero commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

Description

Full screen counterpart to #1509. This adds a new view PaywallView which wraps the Paywall composable so it's usable in the view system.

The view will be used in the hybrids.

@tonidero tonidero changed the base branch from main to add-paywall-footer-view December 1, 2023 10:24
@tonidero tonidero marked this pull request as ready for review December 1, 2023 10:26
@tonidero tonidero requested a review from a team December 1, 2023 10:26
@codecov

codecov Bot commented Dec 1, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9fab167) 84.50% compared to head (0b76428) 84.50%.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           add-paywall-footer-view    #1510   +/-   ##
========================================================
  Coverage                    84.50%   84.50%           
========================================================
  Files                          218      218           
  Lines                         7211     7211           
  Branches                      1004     1004           
========================================================
  Hits                          6094     6094           
  Misses                         729      729           
  Partials                       388      388           

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

@vegaro vegaro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is some code shared between PaywallView and PaywallFooterView. I was wondering if it would be worth it to extract it to a common class. For example, the bug with the font family resource id being 0 could have been fixed in both views at the same time if we had that code somewhere else that's common for both. But I am not sure what's the best strategy to share that code.

@tonidero

tonidero commented Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

There is some code shared between PaywallView and PaywallFooterView. I was wondering if it would be worth it to extract it to a common class. For example, the bug with the font family resource id being 0 could have been fixed in both views at the same time if we had that code somewhere else that's common for both. But I am not sure what's the best strategy to share that code.

Ohh right... There is one more parameter currently in the full screen view than that footer view, and this uses ids, but I will think of ways to share that, thanks!

…venuecatui/views/PaywallView.kt

Co-authored-by: Cesar de la Vega <cesarvegaro@gmail.com>
tonidero and others added 2 commits December 5, 2023 17:46
### Description
This is a possible approach to [this
comment](#1510 (review)).
Basically, there is a bunch of duplicated logic between `PaywallView`
and `PaywallFooterView`. This extracts part of it to a different class,
the logic related to parsing xml attributes.

We could try to create a superclass/interface that abstracts some of the
other methods... But that is relatively straightforward, and didn't want
to have to expose another type just for that.
…d-paywall-view

# Conflicts:
#	ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/views/PaywallFooterView.kt
#	ui/revenuecatui/src/main/res/values/paywall_view_attrs.xml
@vegaro

vegaro commented Dec 5, 2023

Copy link
Copy Markdown
Member

@tonidero can you take a look at the merge conflicts resolution?

@tonidero

tonidero commented Dec 5, 2023

Copy link
Copy Markdown
Contributor Author

Took a look, makes sense! :shipit:

@vegaro vegaro merged commit 07cfe84 into add-paywall-footer-view Dec 5, 2023
@vegaro vegaro deleted the add-paywall-view branch December 5, 2023 17:27
@NachoSoto

Copy link
Copy Markdown
Contributor

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants