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

PageState.targetPage didn't perform as we expected when page offset between -0.5f and 0f. #815

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

MuffinMyHeart
Copy link
Contributor

@MuffinMyHeart MuffinMyHeart commented Oct 23, 2021

Fix the PagerState.targetPage when the currentPageOffset between -0.5f and 0f, the targetPage is currentPage + 1 which is wrong, it should be currentPage - 1.

@jakoss
Copy link

jakoss commented Nov 5, 2021

Can anyone take a look at this? Changes are minor and the impact is quite big. For now the indicator behavior is pretty much unacceptable :)

@andkulikov
Copy link
Collaborator

andkulikov commented Nov 25, 2021

Hey!
I am ready to accept the fix for PagerState if you can remove the change HorizontalPagerWithOffsetTransition from it (ideally if you can also write a simple test for your change). Could you please clarify what do you mean by the picture can't move, as I expect the code using Modifier.offset to work correctly. We also can't just use a constant in pixels like 64 as this will mean it works differently on screens with different densities, that is why 36.dp was used in the original code

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 26, 2021
@MuffinMyHeart
Copy link
Contributor Author

Hey!
I am ready to accept the fix for PagerState if you can remove the change HorizontalPagerWithOffsetTransition from it (ideally if you can also write a simple test for your change). Could you please clarify what do you mean by the picture can't move, as I expect the code using Modifier.offset to work correctly. We also can't just use a constant in pixels like 64 as this will mean it works differently on screens with different densities, that is why 36.dp was used in the original code

In my branch , HorizontalPagerWithOffsetTransition does't work correctly for some reason .But in master branch , the same code works correctly ,and I have reverted the HorizontalPagerWithOffsetTransition.
I don't know how to write test code for this , but I have passed all the test case the pager project provides.
image

@jakoss
Copy link

jakoss commented Nov 26, 2021

What happened here? Is the pager indicator fixed in another pull request?

…f and 0f,the targetPage is currentPage + 1 which is wrong, it should be currentPage - 1.
@MuffinMyHeart MuffinMyHeart reopened this Nov 26, 2021
@MuffinMyHeart MuffinMyHeart changed the title PageState.targetPage didn't perform as we expected when page offset between -0.5f and -0f, and HorizontalPagerTransitionSample profilePicture can't move PageState.targetPage didn't perform as we expected when page offset between -0.5f and 0f. Nov 26, 2021
@MuffinMyHeart
Copy link
Contributor Author

What happened here? Is the pager indicator fixed in another pull request?

Some problems happened ,It took me a while. now I reopen it

@andkulikov andkulikov merged commit ecfbbab into google:main Nov 26, 2021
@andkulikov
Copy link
Collaborator

Thanks

@jakoss
Copy link

jakoss commented Dec 2, 2021

@andkulikov Is there a plan to release bugfix stable-ish version with this?

@andkulikov andkulikov mentioned this pull request Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants