-
Notifications
You must be signed in to change notification settings - Fork 149
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
migration to androidx once more :) #36
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey kibotu, thanks for your contribution and for creating a PR again for this update! I have a couple of questions and suggestions. Let me know what you think.
...agerindicator/src/main/java/com/rbrooks/indefinitepagerindicator/IndefinitePagerIndicator.kt
Outdated
Show resolved
Hide resolved
.../com/rbrooks/indefinitepagerindicatorsample/rtlViewPagerSample/RTLViewPagerSampleFragment.kt
Outdated
Show resolved
Hide resolved
...n/java/com/rbrooks/indefinitepagerindicatorsample/viewPagerSample/ViewPagerSampleFragment.kt
Outdated
Show resolved
Hide resolved
sample/src/test/java/com/rbrooks/indefinitepagerindicatorsample/ExampleUnitTest.kt
Outdated
Show resolved
Hide resolved
...n/java/com/rbrooks/indefinitepagerindicatorsample/viewPagerSample/ViewPagerSampleFragment.kt
Outdated
Show resolved
Hide resolved
i've updated all the points of your feedback @wching |
Testing today and then if nothing is off, I'll merge it @kibotu! :) |
yea let me know :) it's a cool project which i personally believe to be the best long lists dot indicators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey kibotu! Reviewed again and I have a couple of questions :)
implementation "androidx.appcompat:appcompat:1.1.0" | ||
implementation "androidx.appcompat:appcompat-resources:1.1.0" | ||
implementation 'androidx.recyclerview:recyclerview:1.1.0-beta04' | ||
implementation 'com.google.android.material:material:1.1.0-alpha10' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we adding the material alpha here?
and I think we should stick to stable versions for the sake of everybody :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any news @kibotu?
@@ -378,7 +424,7 @@ class IndefinitePagerIndicator @JvmOverloads constructor( | |||
private fun getMostVisibleChild(): View? { | |||
var mostVisibleChild: View? = null | |||
var mostVisibleChildPercent = 0f | |||
for (i in recyclerView?.layoutManager?.childCount!! - 1 downTo 0) { | |||
for (i in (recyclerView?.layoutManager?.childCount ?: 0 - 1).coerceAtLeast(0) downTo 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 to the coerceAtLeast
private fun setIntermediateSelectedItemPosition(mostVisibleChild: View?) { | ||
with(recyclerView?.findContainingViewHolder(mostVisibleChild)?.adapterPosition!!) { | ||
private fun setIntermediateSelectedItemPosition(mostVisibleChild: View) { | ||
with(recyclerView?.findContainingViewHolder(mostVisibleChild)?.adapterPosition) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could remove the with clause and use let instead. Because we are just using this everywhere that way we could also do nothing if not adapter position exists. What do you think?
@kibotu any news? |
Closing as #39 was quicker addressing concerns |
updated