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

[Pager] Migrate to Modifier.scrollable #236

Merged
merged 7 commits into from
Mar 17, 2021
Merged

Conversation

chrisbanes
Copy link
Contributor

@chrisbanes chrisbanes commented Mar 11, 2021

Fixes #247

@google-cla google-cla bot added the cla: yes label Mar 11, 2021
@chrisbanes chrisbanes mentioned this pull request Mar 12, 2021
5 tasks
Base automatically changed from cb/pager to main March 15, 2021 12:02
@chrisbanes chrisbanes added this to In progress in v0.7.0 Mar 15, 2021
@chrisbanes chrisbanes force-pushed the cb/pager-scrollable branch 2 times, most recently from a5e8bf6 to 836f30e Compare March 16, 2021 16:37
@chrisbanes chrisbanes changed the base branch from main to snapshot March 16, 2021 16:40
@chrisbanes chrisbanes changed the title Try out using Modifier.scrollable [Pager] Migrate to Modifier.scrollable Mar 16, 2021
@chrisbanes chrisbanes marked this pull request as ready for review March 16, 2021 17:46
Copy link
Collaborator

@nickbutcher nickbutcher left a comment

Choose a reason for hiding this comment

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

LGTM, a few things that might be clarified/commented.

Our test is still disabled because we need to make sure
we snap to the correct page after an a11y scroll. Most
a11y services will just scroll enough so that the item is
in view port, which isn't how Pager works.
We also speed up the snapping animation via a stiffer
spring
@chrisbanes chrisbanes merged commit 7049e94 into snapshot Mar 17, 2021
v0.7.0 automation moved this from In progress to Done Mar 17, 2021
@chrisbanes chrisbanes deleted the cb/pager-scrollable branch March 17, 2021 09:14
Copy link

@matvei-g matvei-g left a comment

Choose a reason for hiding this comment

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

Thanks Chris, just two smallish comments!

@IntRange(from = 1) offscreenLimit: Int = 1,
content: @Composable PagerScope.(page: Int) -> Unit
decayAnimationSpec: DecayAnimationSpec<Float> = defaultDecayAnimationSpec(),
snapAnimationSpec: AnimationSpec<Float> = spring(stiffness = SnapSpringStiffness),

Choose a reason for hiding this comment

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

Here's is an idea: since your internal PagerState.fling uses only public API available (if not, it's still a good idea :) ) what you can do here is to replace two animation specs with one FlingBehaviour, e.g.

object PagerDefaults {
     @Composable
     fun defaultPagerFlingConfig(state: PagerState, decayAnimationSpec: DecayAnimationSpec<Float>, snapAnimationSpec: AnimationSpec<Float>): FlingConfig { ... }
}

Or similar, then you can just open the whole flingbehaviour for overriding (make FlingBehaviour a param in HorizontalPager), so people can:

  1. Use default behavior by providing nothing
  2. Use default with tweaked params
  3. Use their own sophisticated or slightly different fling behaviour by providing their own instance.
  4. The API for customization will be consistent with LazyColumn and friends.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I like it, I'll follow up after #254

val density = LocalDensity.current
val decay = remember(density) { splineBasedDecay<Float>(density) }
val isRtl = LocalLayoutDirection.current == LayoutDirection.Rtl
val reverseDirection = if (isRtl) !reverseLayout else reverseLayout

Choose a reason for hiding this comment

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

If you do if (isRtl) reverseLayout else !reverseLayout to "reverse the reverse", do you still need the shenanigans with minuses in the rest of the logic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[Pager] Migrate over to Modifier.scrollable()
3 participants