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 V2] Added overflow property to stack #4767

Open
wants to merge 7 commits into
base: paywalls-v2/visible-property
Choose a base branch
from

Conversation

joshdholtz
Copy link
Member

Motivation

We need to allow stacks to have some overflow properties like scroll

Description

Added overflow property to stack with values of:

  • none
  • scroll

@joshdholtz joshdholtz requested review from a team February 6, 2025 01:47
@joshdholtz joshdholtz changed the title [Paywalls V2] Added overflow property to stack [Paywalls V2] Added overflow property to stack Feb 6, 2025
@MarkVillacampa
Copy link
Member

Code looks good, just curious how this will behave with nested scrollviews 😅

@joshdholtz
Copy link
Member Author

Code looks good, just curious how this will behave with nested scrollviews 😅

@MarkVillacampa Yeaaahhh, I think we prevent nesting in the frontend/backend validation... or at least warn about it 😛

Initially we will only offer scrolling on horizontal stacks! And them maybe disabling of scrolling on the root stack 😇

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Looks good! Just some questions

var scrollViewAxis: Axis.Set {
switch self {
case .horizontal: return .horizontal
case .vertical: return .vertical
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure, we won’t allow scrolling in the opposite direction than the axis of the stack then? I think that can make sense for now (in the android PR I was assuming a direction would be given by the backend to make it more flexible… but we might not need that flexibility)

Copy link
Member Author

Choose a reason for hiding this comment

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

@tonidero Correct correct! That might make it a bit more confusing to use 😅

@joshdholtz joshdholtz marked this pull request as ready for review February 13, 2025 11:57
@joshdholtz joshdholtz changed the base branch from main to paywalls-v2/visible-property February 13, 2025 11:57
@joshdholtz joshdholtz requested review from tonidero, JayShortway and a team February 13, 2025 11:58
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Makes sense!

@joshdholtz joshdholtz force-pushed the paywalls-v2/visible-property branch from f8286e8 to 5bc8084 Compare February 13, 2025 19:48
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.

4 participants