Improve fullscreen templates in landscape orientation#1435
Conversation
|
Screenshots change in: RevenueCat/purchases-android-snapshots#3. Will hold this until the base PR is ready. snapshot tests will fail until that PR is merged. |
NachoSoto
left a comment
There was a problem hiding this comment.
Awesome! Just one question.
There was a problem hiding this comment.
What if the paywall isn't displayed full screen and is instead a dialog?
There was a problem hiding this comment.
Hmm I was assuming this would pick the window size of the dialog, not the screen, but I was wrong,
so this change would still apply there. As long as the width of the screen is bigger than the height, we will set the image as a percentage of the screen, instead of using an aspect ratio. This is not ideal, so trying to figure out a solution. Great catch! Thanks!
There was a problem hiding this comment.
I changed the algorithm to use the template size instead of the window size, so we don't care if it's displayed as a dialog, as full screen, or anything the developer wants to do. This will cause a recomposition unfortunately... But I think it's the best solution
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
74ae68b to
b878729
Compare
| aspectRatio(ratio = 1.2f) | ||
| } | ||
| .conditional(aspectRatio <= 1f) { | ||
| fillMaxWidth().height(screenHeight.dp / 2f) |
There was a problem hiding this comment.
Does this keep the aspect ratio? The fillMaxWidth() will fill the parent and the height will be reduced? So a portrait image will look squashed right?
There was a problem hiding this comment.
No it won't. However, we are using ContentScale.Crop here, so the image won't look squashed but it will get cut, so depending on the image it might not look great.
I agree, this might not look great in some scenarios, but usually, this shouldn't be a problem as long as the width is not much larger than the height. So, for developers presenting as a dialog, this shouldn't be an issue I think (the dialog will not use the full available width in these scenarios.
Do you have any suggestions to handle this instead? This is the screenshots change after this: RevenueCat/purchases-android-snapshots#3
There was a problem hiding this comment.
I was missing the ContentScale.Crop 😄
I was worried the image would look squashed, but it won't, so this should be fine
| aspectRatio(ratio = Template5UIConstants.headerAspectRatio) | ||
| } | ||
| .conditional(aspectRatio <= 1f) { | ||
| fillMaxHeight(Template5UIConstants.percentageScreenImageInLandscape).fillMaxWidth() |
There was a problem hiding this comment.
same question here about the aspect ratio
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1435 +/- ##
=======================================
Coverage 84.13% 84.13%
=======================================
Files 197 197
Lines 6649 6649
Branches 965 965
=======================================
Hits 5594 5594
Misses 684 684
Partials 371 371 ☔ View full report in Codecov by Sentry. |
**This is an automatic release.** ### RevenueCatUI * `PaywallActivityLauncher`: new constructor for a generic `ActivityResultCaller` (#1441) via NachoSoto (@NachoSoto) * Improve fullscreen templates in landscape orientation (#1435) via Toni Rico (@tonidero) * `Paywalls`: improve Japanese localization (#1439) via NachoSoto (@NachoSoto) ### Other Changes * Remove side effect from setting purchasesUpdatedListener (#1443) via Cesar de la Vega (@vegaro) * Paywalls: Store paywall events on disk and API (1) (#1436) via Toni Rico (@tonidero) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>
Description
This modifies templates 1 and 5 so the top images don't use the full height of the screen when using devices in horizontal. This is not a problem when presenting as a dialog, since the width is limited, but if using the composables directly, it might be problematic.