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-indicator] Support indicator of looping page with infinite page size #1158

Merged
merged 8 commits into from
May 18, 2022

Conversation

xiaozhikang0916
Copy link
Contributor

Currently pager-indicator use PagerState.pageCount as the size of indicators. When I follow the guide of HorizontalPagerLoopingSample, passing a Int.MAX_VALUE as page count, the indicator crashed, with no doubt.

I add a param size for user who needs to declare a specific page size to show indicators, and a pageMapper lambda to map from giving page index to a proper index.

@google-cla
Copy link

google-cla bot commented May 11, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@xiaozhikang0916
Copy link
Contributor Author

Review needed

Copy link
Collaborator

@andkulikov andkulikov left a comment

Choose a reason for hiding this comment

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

Thank you! I left a few comments

@@ -61,6 +66,8 @@ import androidx.compose.ui.unit.dp
fun HorizontalPagerIndicator(
pagerState: PagerState,
modifier: Modifier = Modifier,
size: Int = pagerState.pageCount,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please rename "size" to pageCount
and "pageMapper" to "pageIndexMapping"

@@ -48,6 +50,9 @@ import androidx.compose.ui.unit.dp
*
* @param pagerState the state object of your [Pager] to be used to observe the list's state.
* @param modifier the modifier to apply to this layout.
* @param size the size of indicators should be displayed, defaults to [PagerState.pageCount].
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be nice to also mention the use case when it is usually needed, I mean this looping scrollable pager

Box(indicatorModifier)
}
}

val position = pageMapper(pagerState.currentPage)
Copy link
Collaborator

Choose a reason for hiding this comment

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

please move all those calculations in the offset block. previously we only need to relayout when the currentPageOffset changes, now we also need to recompose. You can learn more about that here: https://youtu.be/EOQB8PTLkpY?t=434

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, recompose is always a hard part for me to figure out.

Copy link
Collaborator

@andkulikov andkulikov left a comment

Choose a reason for hiding this comment

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

Thanks

@andkulikov
Copy link
Collaborator

The CI is complaining that there are some format violations in HorizontalPagerLoopingIndicatorSample file. Run './gradlew :sample:spotlessApply' to fix these violations.

@xiaozhikang0916
Copy link
Contributor Author

xiaozhikang0916 commented May 17, 2022

The CI is complaining that there are some format violations in HorizontalPagerLoopingIndicatorSample file. Run './gradlew :sample:spotlessApply' to fix these violations.

This might be a bug of spotlessApply. It suggested to remove indents of the lambda block in IconButton, whose arguments were in one line.

I put the arguments in seperate lines, not following the change of :sample:spotlessApply, and check task passed in my local run.

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.

None yet

2 participants