Skip to content
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] Remove lazy stack usages and fix alignment issues #4514

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MarkVillacampa
Copy link
Member

@MarkVillacampa MarkVillacampa commented Nov 22, 2024

Cramped three related things together in this PR:

  • Set the min length of Spacer to zero: otherwise, there is a minimum size that depends on the platform but it's > 0 and could make a view overflow its parent if has fixed size and the sum of its child views + spacers exceeds it.
  • Removed usages of LazyVStack and LazyHStack: they cannot have a child view with width/height type fill because the lazy rendering makes it impossible to know how much space they can occupy. There is not much potential for performance wins since the paywalls should be static and not have a lot of scrolling.
  • Fixed alignment issues to make rendering the view below possible:
Screenshot 2024-11-22 at 17 52 41

3 alignments are needed to achieve this layout:
1- Align green box to the top of the red box
2- Align yellow boxes to the left of the green box.
3- Align yellow boxes to the top within each other.

I added code level comments to clarify what each alignment is doing, and PR comments on the lines where each of these three alignments are performed for extra clarity.

if index < self.componentViewModels.count - 1 {
Spacer()
}
Spacer(minLength: 0)
Copy link
Member Author

@MarkVillacampa MarkVillacampa Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the spaceAround flex layout: each items has one spacer on each side, so there is two spacers between items and one at the start/end.

In contrast, spaceEvenly has one spacer between items, as well as one spacer at the start/end.

HStack(alignment: verticalAlignment.stackAlignment, spacing: style.spacing) {
case .normal:
HStack(
// This alignment positions inner items vertically relative to each other
Copy link
Member Author

@MarkVillacampa MarkVillacampa Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is alignment number 3: Align yellow boxes to the top within each other.

ComponentsView(componentViewModels: self.viewModels, onDismiss: self.onDismiss)
}
// This alignment positions the items horizontally within its parent
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is alignment number 2: Align yellow boxes to the left of the green box.

@@ -76,6 +77,7 @@ struct StackComponentView: View {
viewModels: self.viewModel.viewModels,
onDismiss: self.onDismiss
)
// This alignment positions the inner HStack vertically.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is alignment number 1: Align green box to the top of the red box

return .normal
case .fill, .fixed:
case .spaceBetween, .spaceAround, .spaceEvenly:
return .flex
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flex stacks support .start, .center, .end distribution as well, so we could perfectly remove usages of VStack/HStack and leave just FlexVStack/FlexHStack.

However, the view structure of the latter are a bit more complex/nested than the simple stacks, so I've opted to keep them for now.

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks so good! :shipit:

Side note... we should add the view like you have in the demo into a SwiftUI preview so we can have snapshots on it (at some point) 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants