Fixes for paywall tester #1
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR makes several key changes to the paywall components system, focusing on layout improvements and naming consistency:
- Added new
FlexHStackcomponent with 6 distribution modes (start, center, end, spaceBetween, spaceAround, spaceEvenly) for flexible horizontal layouts - Introduced conditional VStack/LazyVStack usage based on child count (<3) to improve performance with many components
- Fixed incorrect cornerRadius mapping where bottomRight incorrectly used bottomLeading value in StackComponentViewModel
- Renamed font weights for consistency (ultraLight→extraLight, heavy→extraBold) and standardized snake_case raw values
- Renamed property from
componentsConfigstocomponentsConfigthroughout codebase for API consistency
The changes improve layout control and naming consistency while fixing some bugs, though performance concerns with VStack/LazyVStack are noted as not final.
17 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
| ForEach(0..<componentViewModels.count, id: \.self) { index in | ||
| ComponentsView(componentViewModels: [self.componentViewModels[index]], onDismiss: self.onDismiss) | ||
| } |
There was a problem hiding this comment.
logic: Using array index directly as id could cause issues if items change. Consider using a more stable identifier from PaywallComponentViewModel
| ForEach(0..<componentViewModels.count, id: \.self) { index in | ||
| Spacer() | ||
| ComponentsView(componentViewModels: [self.componentViewModels[index]], onDismiss: self.onDismiss) | ||
| } | ||
| Spacer() |
There was a problem hiding this comment.
logic: spaceEvenly implementation places Spacer before first item, which differs from CSS flex justify-content: space-evenly. Should have equal space between all items
| // There are some horizontal sizing issues with using LazyVStack | ||
| // There are so performance issues with VStack with lots of children |
There was a problem hiding this comment.
syntax: typo in comment: 'There are so performance issues' should be 'There are some performance issues'
| // This is NOT a final implementation of this | ||
| // There are some horizontal sizing issues with using LazyVStack | ||
| // There are so performance issues with VStack with lots of children | ||
| if viewModel.shouldUseVStack { |
There was a problem hiding this comment.
logic: using a hardcoded threshold of 3 items (from ViewModel) to switch between VStack and LazyVStack could cause performance issues with complex child views even with few items
| if viewModels.count < 3 { | ||
| return true | ||
| } | ||
| return false | ||
| case .horizontal, .zlayer: |
There was a problem hiding this comment.
style: Simplify to return viewModels.count < 3 since both paths return a boolean
| let alignment = try container.decode(VerticalAlignment.self, forKey: .alignment) | ||
| self = .horizontal(alignment) | ||
| let distribution = try container.decode(FlexDistribution.self, forKey: .distribution) | ||
| self = .horizontal(alignment, distribution) |
There was a problem hiding this comment.
logic: decoding will fail for existing horizontal dimensions that don't have distribution field - need migration strategy
| enum FontWeight: String, Codable, Sendable, Hashable, Equatable { | ||
|
|
||
| case ultraLight | ||
| case extraLight = "extra_light" | ||
| case thin | ||
| case light | ||
| case regular | ||
| case medium | ||
| case semibold | ||
| case bold | ||
| case heavy | ||
| case extraBold = "extra_bold" | ||
| case black | ||
|
|
||
| } |
There was a problem hiding this comment.
logic: FontWeight case changes from ultraLight to extraLight and heavy to extraBold will break existing paywall configurations that use the old values
| case type | ||
| case text = "text_lid" | ||
| case text = "textLid" | ||
| case fontFamily |
There was a problem hiding this comment.
logic: this change from text_lid to textLid will break deserialization of existing paywall data that uses the old key format. Need to either maintain backward compatibility or document migration steps
| .sheet(item: self.$presentedPaywall) { paywall in | ||
| #if PAYWALL_COMPONENTS | ||
| if let componentData = paywall.offering.paywallComponentsData { | ||
| // TODO: Get locale | ||
| TemplateComponentsView( | ||
| paywallComponentsData: componentData, | ||
| offering: paywall.offering, | ||
| onDismiss: { self.presentedPaywall = nil } | ||
| ) | ||
| } else { | ||
| PaywallPresenter(offering: paywall.offering, mode: paywall.mode, introEligility: .eligible) | ||
| .onRestoreCompleted { _ in | ||
| self.presentedPaywall = nil | ||
| } | ||
| } | ||
| #else | ||
| PaywallPresenter(offering: paywall.offering, mode: paywall.mode, introEligility: .eligible) | ||
| .onRestoreCompleted { _ in | ||
| self.presentedPaywall = nil | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
logic: Removing PAYWALL_COMPONENTS conditional breaks component-based paywall presentation. Should keep the conditional to support both standard and component-based paywalls.
| if name == "components" { | ||
| return "V2" | ||
| } else if let template = PaywallTemplate(rawValue: name) { |
There was a problem hiding this comment.
style: Hardcoding 'V2' for components template name reduces clarity. Consider using a more descriptive constant or enum value.
Checklist
purchases-androidand hybridsMotivation
Description