-
Notifications
You must be signed in to change notification settings - Fork 54
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
[Paywalls V2] Add Badge
's overlay
style layout
#2009
Conversation
26fb5c4
to
ca27f5d
Compare
state, | ||
modifier = Modifier | ||
.align(alignment.toAlignment()) | ||
.onGloballyPositioned { |
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 don't really like using this, since emerge/snapshot tests usually don't pick these changes correctly... Not sure if there is a better way to do this, while allowing arbitrary contents inside the badge though...
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.
Generally influencing the layout of a composable can be done in a .layout { }
modifier. That allows us to keep layout logic in the layout phase, and avoids an extra composition. Instead of .onGloballyPositioned { }
, we can do something like this:
.layout { measurable, constraints ->
val placeable = measurable.measure(constraints)
layout(placeable.width, placeable.height) {
placeable.placeRelative(
x = getOverlaidBadgeOffsetX(placeable.width, alignment),
y = getOverlaidBadgeOffsetY(placeable.height, alignment),
)
}
}
with:
private fun getOverlaidBadgeOffsetX(width: Int, alignment: TwoDimensionalAlignment) =
when (alignment) {
TwoDimensionalAlignment.CENTER,
TwoDimensionalAlignment.TOP,
TwoDimensionalAlignment.BOTTOM -> 0
TwoDimensionalAlignment.LEADING,
TwoDimensionalAlignment.TOP_LEADING,
TwoDimensionalAlignment.BOTTOM_LEADING -> (-(width.toFloat() / 2)).roundToInt()
TwoDimensionalAlignment.TRAILING,
TwoDimensionalAlignment.TOP_TRAILING,
TwoDimensionalAlignment.BOTTOM_TRAILING -> (width.toFloat() / 2).roundToInt()
}
private fun getOverlaidBadgeOffsetY(height: Int, alignment: TwoDimensionalAlignment) =
when (alignment) {
TwoDimensionalAlignment.CENTER,
TwoDimensionalAlignment.LEADING,
TwoDimensionalAlignment.TRAILING -> 0
TwoDimensionalAlignment.TOP,
TwoDimensionalAlignment.TOP_LEADING,
TwoDimensionalAlignment.TOP_TRAILING -> (-(height.toFloat() / 2)).roundToInt()
TwoDimensionalAlignment.BOTTOM,
TwoDimensionalAlignment.BOTTOM_LEADING,
TwoDimensionalAlignment.BOTTOM_TRAILING -> (height.toFloat() / 2).roundToInt()
}
(We should then also remove the padding from the MainStackComponent
.)
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.
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.
Actually it's just making getOverlaidBadgeOffsetX
be 0 always :P
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.
Did this in 3215712
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.
Oh of course! For some reason I thought that was the requirement haha 😅
StackComponentView( | ||
badgeStack, | ||
state, | ||
modifier = Modifier |
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'm wondering if we should automatically add some start/end paddings if the "root" stack has rounded corners (as it is in the designs), or just rely on given margins for the "badge's stack". This would mean we need to add those margins from the backend, on the other hand, it's more flexible, since less logic in the SDK.
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 think we should do it on the backend, for the flexibility reason you mention + otherwise it becomes very hard to reason about SDK behavior. Also, that would be in line with our "a badge is actually a stack" approach. The backend should provide all properties to be able to render that badge stack correctly imo, just as it does for normal stacks.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2009 +/- ##
=======================================
Coverage 81.88% 81.89%
=======================================
Files 260 261 +1
Lines 8514 8524 +10
Branches 1226 1227 +1
=======================================
+ Hits 6972 6981 +9
Misses 1043 1043
- Partials 499 500 +1 ☔ View full report in Codecov by Sentry. |
# Conflicts: # ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/button/ButtonComponentView.kt # ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/stack/StackComponentView.kt # ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/style/StackComponentStyle.kt # ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/style/StyleFactory.kt # ui/revenuecatui/src/test/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/button/ButtonComponentViewTests.kt
# Conflicts: # ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/stack/StackComponentView.kt
@tonidero I fixed conflicts by merging instead of rebasing and force pushing, so you can squash, revert or do whatever you want with my commits. 😄 |
.../main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/stack/StackComponentView.kt
Outdated
Show resolved
Hide resolved
.../main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/stack/StackComponentView.kt
Show resolved
Hide resolved
📸 Snapshot Test6 added, 137 unchanged
🛸 Powered by Emerge Tools |
@@ -108,6 +108,9 @@ internal class StackComponentState( | |||
@get:JvmSynthetic | |||
val shadow by derivedStateOf { presentedPartial?.partial?.shadow ?: style.shadow } | |||
|
|||
@get:JvmSynthetic | |||
val badge by derivedStateOf { style.badge } |
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.
So I went a lot of back and forth on this... I think it's fine even if this doesn't use the presentedPartial
? The overrides on the inner stack would already be applied in the style itself I believe. The only thing that wouldn't be overriden is the style and alignment, but not sure if it's worth the refactor that would be required to support that... Lmk if I misunderstood something though!
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 think we should support overrides for the style and alignment 🤔 I can imagine someone wanting to change the badge style on a larger screen, for instance.
Maybe we can separate out the style, alignment and content like so. Maybe that's easier?
@get:JvmSynthetic
val badgeAlignment by derivedStateOf { presentedPartial?.partial?.badge?.alignment ?: style.badge?.alignment }
@get:JvmSynthetic
val badgeStyle by derivedStateOf { presentedPartial?.partial?.badge?.style ?: style.badge?.style }
@get:JvmSynthetic
val badgeContent = style.badge?.stackStyle
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 ended up changing it to override the style and alignment in a single field by creating a new instance of the style if needed: 6d0d6ee. This makes it simpler since we only have a single nullable field instead of 3. Lmk what you think!
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 ... think this is perfect? Nice one!
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 like it a lot! Just the 2 comments on the overrides and the layout modifier.
@@ -108,6 +108,9 @@ internal class StackComponentState( | |||
@get:JvmSynthetic | |||
val shadow by derivedStateOf { presentedPartial?.partial?.shadow ?: style.shadow } | |||
|
|||
@get:JvmSynthetic | |||
val badge by derivedStateOf { style.badge } |
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 think we should support overrides for the style and alignment 🤔 I can imagine someone wanting to change the badge style on a larger screen, for instance.
Maybe we can separate out the style, alignment and content like so. Maybe that's easier?
@get:JvmSynthetic
val badgeAlignment by derivedStateOf { presentedPartial?.partial?.badge?.alignment ?: style.badge?.alignment }
@get:JvmSynthetic
val badgeStyle by derivedStateOf { presentedPartial?.partial?.badge?.style ?: style.badge?.style }
@get:JvmSynthetic
val badgeContent = style.badge?.stackStyle
state, | ||
modifier = Modifier | ||
.align(alignment.toAlignment()) | ||
.onGloballyPositioned { |
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.
Generally influencing the layout of a composable can be done in a .layout { }
modifier. That allows us to keep layout logic in the layout phase, and avoids an extra composition. Instead of .onGloballyPositioned { }
, we can do something like this:
.layout { measurable, constraints ->
val placeable = measurable.measure(constraints)
layout(placeable.width, placeable.height) {
placeable.placeRelative(
x = getOverlaidBadgeOffsetX(placeable.width, alignment),
y = getOverlaidBadgeOffsetY(placeable.height, alignment),
)
}
}
with:
private fun getOverlaidBadgeOffsetX(width: Int, alignment: TwoDimensionalAlignment) =
when (alignment) {
TwoDimensionalAlignment.CENTER,
TwoDimensionalAlignment.TOP,
TwoDimensionalAlignment.BOTTOM -> 0
TwoDimensionalAlignment.LEADING,
TwoDimensionalAlignment.TOP_LEADING,
TwoDimensionalAlignment.BOTTOM_LEADING -> (-(width.toFloat() / 2)).roundToInt()
TwoDimensionalAlignment.TRAILING,
TwoDimensionalAlignment.TOP_TRAILING,
TwoDimensionalAlignment.BOTTOM_TRAILING -> (width.toFloat() / 2).roundToInt()
}
private fun getOverlaidBadgeOffsetY(height: Int, alignment: TwoDimensionalAlignment) =
when (alignment) {
TwoDimensionalAlignment.CENTER,
TwoDimensionalAlignment.LEADING,
TwoDimensionalAlignment.TRAILING -> 0
TwoDimensionalAlignment.TOP,
TwoDimensionalAlignment.TOP_LEADING,
TwoDimensionalAlignment.TOP_TRAILING -> (-(height.toFloat() / 2)).roundToInt()
TwoDimensionalAlignment.BOTTOM,
TwoDimensionalAlignment.BOTTOM_LEADING,
TwoDimensionalAlignment.BOTTOM_TRAILING -> (height.toFloat() / 2).roundToInt()
}
(We should then also remove the padding from the MainStackComponent
.)
StackComponentView( | ||
badgeStack, | ||
state, | ||
clickHandler, |
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 we can create a Badge
composable here that takes a TwoDimensionalAlignment
and a Badge.Style
, and is implemented as a StackComponentView
with the right (layout) modifier? That might simplify this file a bit.
Alternatively, as a first step we could create an OverlaidBadge
composable that does the same thing, but only implements the Overlay
style, and just takes a TwoDimensionalAlignment
.
Just a suggestion. Let me know what you think!
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.
Done in 1129ef4
StackComponentView( | ||
badgeStack, | ||
state, | ||
modifier = Modifier |
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 think we should do it on the backend, for the flexibility reason you mention + otherwise it becomes very hard to reason about SDK behavior. Also, that would be in line with our "a badge is actually a stack" approach. The backend should provide all properties to be able to render that badge stack correctly imo, just as it does for normal stacks.
.../main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/stack/StackComponentView.kt
Show resolved
Hide resolved
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.
Looks amazing! And thanks for making the changes!
Box(modifier = modifier) { | ||
MainStackComponent(stackState, state, clickHandler, selected = selected) | ||
OverlaidBadge(badgeStack, state, alignment, selected = selected) | ||
} |
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.
Beautiful!
state, | ||
modifier = Modifier | ||
.align(alignment.toAlignment()) | ||
.onGloballyPositioned { |
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.
Oh of course! For some reason I thought that was the requirement haha 😅
@@ -108,6 +108,9 @@ internal class StackComponentState( | |||
@get:JvmSynthetic | |||
val shadow by derivedStateOf { presentedPartial?.partial?.shadow ?: style.shadow } | |||
|
|||
@get:JvmSynthetic | |||
val badge by derivedStateOf { style.badge } |
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 ... think this is perfect? Nice one!
…le layout (#2039) ### Description Follow-up to #2009 This PR adds the `edgeToEdge` top/bottom style for badges. <img width="911" alt="image" src="https://github.com/user-attachments/assets/a2722e94-8bd1-4867-a8a6-0edfceac2ed6" /> --------- Co-authored-by: JayShortway <29483617+JayShortway@users.noreply.github.com>
### Description Followup to #2009 and #2039. This adds support for `nested` style badges. As discussed over Slack, the nested badges won't move the content of the stack, and just be placed on top. <img width="906" alt="image" src="https://github.com/user-attachments/assets/4cc2e2d9-a336-4890-b40a-a065b247a1b7" />
**This is an automatic release.** ## RevenueCat SDK ### ✨ New Features * Add `subscriptionsByProductIdentifier` to `CustomerInfo` (#2052) via Cesar de la Vega (@vegaro) ### 🐞 Bugfixes * Fix `OwnershipType` enum serialization (#2061) via Cesar de la Vega (@vegaro) ## RevenueCatUI SDK ### 🐞 Bugfixes * Allow repurchasing custom packages (#2044) via Toni Rico (@tonidero) ### 🔄 Other Changes * [Paywalls V2] Do not attempt to purchase if currently subscribed (#2062) via JayShortway (@JayShortway) * [Trusted Entitlements] Enable `Trusted Entitlements` by default (#2050) via Toni Rico (@tonidero) * [Trusted Entitlements] Do not clear CustomerInfo upon enabling Trusted Entitlements (#2049) via Toni Rico (@tonidero) * [Paywalls V2] Removes `MaskShape.Pill` in favor of `MaskShape.Circle`. (#2063) via JayShortway (@JayShortway) * [Paywalls V2] Font sizes are integers now. (#2059) via JayShortway (@JayShortway) * [Paywalls V2] Handles intro offer eligibility overrides (#2058) via JayShortway (@JayShortway) * [Paywalls V2] Implements `Convex` and `Concave` image masks (#2055) via JayShortway (@JayShortway) * [Paywalls V2] Add new `ImageComponent` properties (#2056) via Toni Rico (@tonidero) * [Paywalls V2] Add `Badge`'s `nested` style layout (#2041) via Toni Rico (@tonidero) * [Paywalls V2] Add `Badge`'s `edgeToEdge` `Top`/`Bottom` alignment style layout (#2039) via Toni Rico (@tonidero) * [Paywalls V2] Various `PaywallViewModel` fixes and tests (#2051) via JayShortway (@JayShortway) * [Paywalls V2] Fixes minimum spacing when distribution is `SPACE_BETWEEN`, `SPACE_AROUND` or `SPACE_EVENLY` (#2053) via JayShortway (@JayShortway) * [Paywalls V2] Correctly determines when to show or hide decimals for prices (#2048) via JayShortway (@JayShortway) * [Paywalls V2] `TextComponentView` uses the correct `Package` for variable values (#2042) via JayShortway (@JayShortway) * [Paywalls V2] Adds Custom Tabs to support in-app browser URL destinations (#2035) via JayShortway (@JayShortway) * Update `agp` to 8.8.0 (#2045) via Toni Rico (@tonidero) * [Paywalls V2] Add `Badge`'s `overlay` style layout (#2009) via Toni Rico (@tonidero) * [Paywalls V2] Implements all button actions (#2034) via JayShortway (@JayShortway) * Convert error message property into computed property (#2038) via Toni Rico (@tonidero) Co-authored-by: revenuecat-ops <ops@revenuecat.com>
Description
This adds the layout for
Badge
to display correctly with theoverlay
layout.This PR moves the existing Stack layout inside another composable:
MainStackComponent
. And at the top-level, we now decide what layout to use depending on the badge style. In this PR, we are implementing the first one:overlay
.