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

Fix: Upward scroll fling value not consumed when refresh callback is called #16

Merged
merged 2 commits into from
Aug 6, 2024
Merged

Fix: Upward scroll fling value not consumed when refresh callback is called #16

merged 2 commits into from
Aug 6, 2024

Conversation

LakeLab
Copy link
Contributor

@LakeLab LakeLab commented Jul 23, 2024

This PR is my revision of #15.
I would appreciate feedback from the maintainers.

LakeLab added 2 commits July 23, 2024 18:07
… called

Consuming the fling value when the refresh starts since it holds no significant meaning in this context.

#15
@LakeLab LakeLab changed the title Fix: Consume Upward Fling Value During Refresh to Prevent Unexpected Scroll Fix: Upward scroll fling value not consumed when refresh callback Is called Jul 23, 2024
@LakeLab LakeLab changed the title Fix: Upward scroll fling value not consumed when refresh callback Is called Fix: Upward scroll fling value not consumed when refresh callback is called Jul 23, 2024
@LakeLab
Copy link
Contributor Author

LakeLab commented Jul 31, 2024

@wingio, Could you help me check whether this is acceptable?

@wingio
Copy link
Member

wingio commented Jul 31, 2024

@wingio, Could you help me check whether this is acceptable?

yeah i could do that tomorrow, and im sorry for not getting to it for so long, i wasn't aware this was opened

@LakeLab
Copy link
Contributor Author

LakeLab commented Jul 31, 2024

For your information, I can revert the code for the sample module. I just wanted to show you the case which is #15 issue.

@LakeLab
Copy link
Contributor Author

LakeLab commented Jul 31, 2024

yeah i could do that tomorrow, and im sorry for not getting to it for so long, i wasn't aware this was opened

No problem! Thank you!

@LakeLab
Copy link
Contributor Author

LakeLab commented Aug 6, 2024

@wingio
pleaseee

@wingio
Copy link
Member

wingio commented Aug 6, 2024

LGTM

@wingio wingio merged commit a668f19 into MateriiApps:release Aug 6, 2024
1 check passed
wingio added a commit that referenced this pull request Aug 6, 2024
…called (#16) (#17)

* Fix the wrong compiler version for the demo project.

* [#15] Upward scroll fling value not consumed when refresh callback Is called

Consuming the fling value when the refresh starts since it holds no significant meaning in this context.

#15

Co-authored-by: Lake <[email protected]>
@LakeLab LakeLab deleted the feature/consume-fling-when-refresh-starts branch August 6, 2024 06:41
.verticalScroll(rememberScrollState()),
contentAlignment = Alignment.Center
LazyColumn(
Modifier.fillMaxSize(),
Copy link
Member

Choose a reason for hiding this comment

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

We use named arguments

Suggested change
Modifier.fillMaxSize(),
modifier = Modifier.fillMaxSize(),

color = MaterialTheme.colorScheme.outline,
shape = RoundedCornerShape(10.dp)
)
.padding(10.dp), text = "No. $it"
Copy link
Member

@X1nto X1nto Aug 6, 2024

Choose a reason for hiding this comment

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

These shouldn't be on the same line

// If the velocity is negative, the fling is upwards, and we don't want to prevent the
// the list from scrolling
velocity < 0f -> 0f
velocity < 0f -> if (isRefreshTiming) {
Copy link
Member

Choose a reason for hiding this comment

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

A one-liner arrow for a multiline if seems weird, use blocks instead

Suggested change
velocity < 0f -> if (isRefreshTiming) {
velocity < 0f -> {
if (isRefreshTiming) {

velocity < 0f -> if (isRefreshTiming) {
// We need to prevent the fling upward when the refresh starts.
velocity
}else {
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace

Suggested change
}else {
} else {

@X1nto
Copy link
Member

X1nto commented Aug 6, 2024

Oh crap I had this tab open like 6 hours ago, forgot to press "Request changes". @wingio could you review code style too next time?

wingio pushed a commit that referenced this pull request Dec 27, 2024
…ode to match with README.md (#19)

* A one-liner arrow for a multiline if seems weird, using blocks instead
    * #16 (comment)
* Add a whitespace
    * #16 (comment)
* Change back DragRefreshSample.kt code to match with README.md
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.

3 participants