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

Don't use the nested scroll view in Column #2555

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

squarejesse
Copy link
Contributor

This breaks overscroll.

I looked into the PR that added this, which was an attempt to work-around a problem where a RecyclerView was nested in a scrollable column. We shouldn't ever do that, so this work-around should be unnecessary.


  • CHANGELOG.md's "Unreleased" section has been updated, if applicable.

@squarejesse
Copy link
Contributor Author

Related: #746

This breaks overscroll.

I looked into the PR that added this, which was an attempt
to work-around a problem where a RecyclerView was nested
in a scrollable column. We shouldn't ever do that, so this
work-around should be unnecessary.
@swankjesse swankjesse force-pushed the jwilson.0207.not_nested branch from 7294526 to 5f51f50 Compare February 7, 2025 21:24
Copy link
Collaborator

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

If an NSV isn't overscrolling it would be because a parent reports itself as being able to consume the scroll events. Is something else scrollable up in the hierarchy? I will still approve, but since NSV is used everywhere normally anything like this should be some other problem that we created.

@squarejesse
Copy link
Contributor Author

Lemme figure out what’s happening and follow-up here.

@JakeWharton
Copy link
Collaborator

We're unlikely to actually need NSV, unless we attempt something like a collapsing header component. So it's not a huge deal to switch for now. Just was a surprising culprit.

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