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

Discussion #722 Nested scroll and overscroll #730

Closed

Conversation

j2esu
Copy link

@j2esu j2esu commented May 26, 2024

Screen_recording_20240526_173848.mp4

PR for the discussion #722

Copy link
Member

@Gowsky Gowsky left a comment

Choose a reason for hiding this comment

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

Hello, thank you for your contribution. The over-scroll effect is a welcome addition, but it should affect just the scrollable content, not the whole composable. y-axes should not be stretched horizontally, for example. Also, both UI frameworks must have the same functionality, so the view system should also receive the over-scroll effect.

}

public class VicoNestedScroll @OptIn(ExperimentalFoundationApi::class)
Copy link
Member

Choose a reason for hiding this comment

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

The name of this class contains "nested scroll", however, it has nothing to do with nested scrolling. The class only defines OverscrollEffect and its enabled state. Thus the name should be changed to e.g. VicoOverscrollEffect.

Comment on lines +267 to +268
public fun rememberDefaultVicoNestedScroll(applyOverscroll: Boolean = true): VicoNestedScroll {
val effect = ScrollableDefaults.overscrollEffect()
Copy link
Member

Choose a reason for hiding this comment

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

The effect should also be the function parameter with a default value.

Suggested change
public fun rememberDefaultVicoNestedScroll(applyOverscroll: Boolean = true): VicoNestedScroll {
val effect = ScrollableDefaults.overscrollEffect()
public fun rememberDefaultVicoNestedScroll(effect = ScrollableDefaults.overscrollEffect(), applyOverscroll: Boolean = true): VicoNestedScroll {

} else {
val consumedValue = value - oldValue
pointerXDeltas.tryEmit(consumedValue - delta)
if (nestedScroll != null) consumedValue else delta
Copy link
Member

Choose a reason for hiding this comment

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

VicoNestedScroll being null should not determine whether nested scroll is enabled. It should be a separate boolean.

@Gowsky Gowsky force-pushed the master branch 2 times, most recently from bb88521 to ac44341 Compare June 1, 2024 20:16
@Gowsky
Copy link
Member

Gowsky commented Jun 21, 2024

Closing as per #722 (reply in thread).

@Gowsky Gowsky closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants