-
Notifications
You must be signed in to change notification settings - Fork 338
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] Add Timeline component #4713
Conversation
@@ -56,9 +56,8 @@ struct IconComponentView: View { | |||
shadow: style.iconBackgroundShadow, | |||
background: style.iconBackgroundStyle, | |||
uiConfigProvider: self.viewModel.uiConfigProvider) | |||
.padding(style.padding) | |||
.padding(style.margin) |
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.
Bug fix: we're already applying the padding a few lines above. This is the margin but we were using the padding property.
.size(style.size) | ||
.clipped() |
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 can't see a reason why the icon needs to be clipped. This is likely to clip the shadow if present, which is not intended.
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.
Copy/paste from image 🫠
@@ -153,7 +152,7 @@ struct IconComponentView_Previews: PreviewProvider { | |||
), | |||
size: .init(width: .fixed(150), height: .fixed(150)), | |||
padding: .init(top: 20, bottom: 20, leading: 20, trailing: 20), | |||
margin: .zero, | |||
margin: .init(top: 20, bottom: 20, leading: 20, trailing: 20), |
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.
Re-adding the margin so the snapshot stays the same
@@ -78,7 +78,6 @@ struct PreviewRequiredEnvironmentProperties: ViewModifier { | |||
let screenCondition: ScreenCondition | |||
let componentViewState: ComponentViewState | |||
let packageContext: PackageContext? | |||
let colorScheme: ColorScheme |
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.
colorScheme
is always present in the environment as it's a default SwiftUI property. Hardcoding it in previewRequiredEnvironmentProperties
was making it impossible to switch the color scheme in Xcode previews.
1 build increased size
Paywalls 1.0 (1)
|
Item | Install Size Change |
---|---|
DYLD.String Table | ⬆️ 151.7 kB |
Code Signature | ⬆️ 7.9 kB |
DYLD.Exports | ⬆️ 5.3 kB |
📝 RevenueCat.PaywallComponent.TimelineComponent.TimelineComponent | ⬆️ 5.1 kB |
📝 RevenueCat.PaywallComponent.TimelineComponent.Connector.Connector | ⬆️ 2.7 kB |
🛸 Powered by Emerge Tools
Comment trigger: Size diff threshold of 100.00kB exceeded
@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *) | ||
extension Shape { | ||
|
||
func fillColorScheme( |
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 already have helpers to apply a foreground and background color scheme but I needed to fill
a shape.
At some point we need to refactor and unify the logic of all these methods.
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.
A few minor comments but this is looking gooooood 🤩
var description: TextComponentViewModel? | ||
if let descriptionComponent = item.description { | ||
description = try TextComponentViewModel( | ||
localizationProvider: localizationProvider, | ||
uiConfigProvider: uiConfigProvider, | ||
component: descriptionComponent | ||
) | ||
} |
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: can flatMap (errr, compactMap) the optional too 🤷♂️
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.
Not sure I follow what you mean here 🤔
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.
Nice! This lgtm! Just the existing comments
public let iconAlignment: IconAlignment? | ||
public let itemSpacing: CGFloat? | ||
public let textSpacing: CGFloat? | ||
public let columnGutter: CGFloat? |
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 interesting... In android I was considering all these mandatory... I guess we could default to 0 in the SDK, but seems like something the backend should always provide? Wdyt?
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 just made them optional out of abundance of caution 😅 I'd expect the backend to send a default value always yes.
Because of how Decodable works in Swift, I cannot specify a default value, I must define them as optional tho.
width: connector.width, | ||
height: proxy[to][.center].y - proxy[from][.center].y - | ||
(item.component.connector?.margin.bottom ?? 0) - | ||
(item.component.connector?.margin.top ?? 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.
Hmm right... I think I will need to do some of this calculations in Android as well in cases where one item is bigger than the others 👍
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.
Adds a new Timeline component which displays a list of items with a title, description, icon and a connector between the icons.