-
Notifications
You must be signed in to change notification settings - Fork 405
Add Customer Center and Paywalls tabs to SampleCat #5989
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
Conversation
Generated by 🚫 Danger |
|
@RevenueCat/coresdk @polpielladev @hiddevdploeg bump on this |
Ooops! Completely missed this, reviewing now 👀 |
| VStack(alignment: .leading, spacing: 4) { | ||
| Text(offering.paywall?.templateName ?? offering.identifier) | ||
| .font(.headline) | ||
| Text("\(offering.availablePackages.count) package\(offering.availablePackages.count > 1 ? "s" : "")") |
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.
nit: but I think we can just use string inflection instead here then we don't have to do the manual pluralization afaik
https://nilcoalescing.com/blog/HandlePluralsInSwiftUITextViewsWithInflection/
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 nice! TIL
polpielladev
left a comment
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.
Overall looks great to me, thanks for doing this @aboedo 🙌
I left a couple of small comments regarding duplication.
| } | ||
| .scrollContentBackground(.hidden) | ||
| .background { | ||
| ContentBackgroundView(color: Color("RC-royal-blue")) |
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.
We have added a new RC-royal-blue on this PR but it has the same hex value as RC-blue, do we want to use that one instead here? Or just remove one in favor of the other duplicate?
tonidero
left a comment
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.
Just a more general question, but not a blocker for this I think. It looks great to me, thanks!
| .padding() | ||
| .blur(radius: healthViewModel.isfetchingHealthReport ? 5 : 0) | ||
| .overlay { | ||
| if healthViewModel.isfetchingHealthReport { |
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.
Hmm it's a bit strange for me to display the loading screen here until the health view model is done. As in, it doesn't seem related... I can see that we do this in other tabs, and I guess it's a way to not have to wait on other more specific pieces of data, like whether the CustomerInfo has loaded... So I guess I wouldn't consider this a blocker right now. It could be confusing though for users using this as the base though I think.
|
Feels like we still have some work to do when it comes to the CI tests reliability |
Summary
ScreenRecording_01-02-2026.17-29-52_1.MP4
Changes
CustomerCenterTabViewwith dark purple (rgb(31, 31, 71)) gradientPaywallsTabViewwith royal blue (rgb(87, 108, 219)) gradientContentViewto include new tabs