-
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 nested
style layout
#2041
Conversation
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 will really like to test this again once the option is in the dashboard. For now, on the previews, this seems to be working.
@@ -73,3 +73,31 @@ internal fun TwoDimensionalAlignment.toHorizontalAlignmentOrNull(): Alignment.Ho | |||
TwoDimensionalAlignment.BOTTOM_LEADING -> Alignment.Start | |||
TwoDimensionalAlignment.BOTTOM_TRAILING -> Alignment.End | |||
} | |||
|
|||
@JvmSynthetic | |||
internal fun TwoDimensionalAlignment.toVerticalAlignment(): Alignment.Vertical = |
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.
This is very similar to toVerticalAlignmentOrNull
but I needed one that assumed a vertical alignment for the "center" types. Same for the horizontal version.
-> cornerRadiuses.copy(topLeading = 0.0, topTrailing = 0.0) | ||
val filteredCornerRadiuses = when (alignment.toVerticalAlignment()) { | ||
Alignment.Top -> cornerRadiuses.copy(bottomLeading = 0.0, bottomTrailing = 0.0) | ||
Alignment.Bottom -> cornerRadiuses.copy(topLeading = 0.0, topTrailing = 0.0) |
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.
These are just some cleanup from just using the respective alignment type.
📸 Snapshot Test6 added, 167 unchanged
🛸 Powered by Emerge Tools |
8d7f0cf
to
a9bc463
Compare
030f0e7
to
ac24767
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2041 +/- ##
=======================================
Coverage 81.51% 81.51%
=======================================
Files 265 265
Lines 8652 8652
Branches 1229 1229
=======================================
Hits 7053 7053
Misses 1098 1098
Partials 501 501 ☔ View full report in Codecov by Sentry. |
ac24767
to
78493cd
Compare
78493cd
to
4d91ce9
Compare
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.
Very nice! Just 1 comment on structure.
Box(modifier = modifier.then(commonModifier)) { | ||
stack(Modifier) | ||
StackComponentView( | ||
nestedBadge.stackStyle, | ||
state, | ||
{}, | ||
modifier = Modifier | ||
.align(nestedBadge.alignment.toAlignment()), | ||
) | ||
} |
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.
Could we move this to the StackComponentView
at the top, inside the when
checking the Badge.Style
?
Badge.Style.Nested -> {
Box(modifier = modifier) {
MainStackComponent(stackState, state, clickHandler, modifier)
StackComponentView(
badge.stackStyle,
state,
{},
modifier = Modifier
.align(badge.alignment.toAlignment()),
)
}
}
That way we don't need to add a nestedBadge
parameter to the MainStackComponent
. That parameter is slightly confusing imo, as it doesn't follow the implementation pattern of the other badges.
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.
Right the main reason for doing it this way is that we have to apply the commonModifier
to the parent Box
, not the MainStackComponent
in this scenario... I agree it's a bit confusing... I'm trying to think of alternatives
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.
Wdyt of this approach @JayShortway ?
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.
Thanks for explaining!
I think I prefer the nestedBadge
parameter approach then. 😳 Having 2 modifier parameters is more confusing imo. Sorry for the back and forth! 🙏
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.
Np! I was also back and forth on this 😅 Will revert that 👍
Seems that the stack's border is drawn on top of the badge, which is different from leading/trailing edge-to-edge badges. Is that intentional? Just wanted to double check. |
Yeah, with nested, it should be "inside" the stack itself, vs the edge-to-edge style which is considered "outside" the badge. If the backend gives us any margin, it would be added within the main stack. |
1409d50
to
c24ba2b
Compare
This reverts commit c24ba2b.
**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
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.