-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix small issues left in version 4.2.1 #1495
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
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.
Self-review to help out.
@@ -92,6 +102,16 @@ public int getPositionForSection(int index) { | |||
|
|||
@Override | |||
public int getSectionForPosition(int position) { | |||
if (sections == null) { |
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.
Since we were using "fast scrolling" this method had to be implemented to get the scroll bar to work correctly.
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.
I tested this, and it seems a bit jumpy. If you scroll all the way down, and go back up, it immediately jumps to a quarter of the way up. If you can fix that, it'd be perfect. Otherwise, still a big improvement over what we had.
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.
The scrollbar also doesn't show up unless we start scrolling - is that the intended behavior?
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.
Yeah won't show up till you start scrolling, that's normal. There's nothing I can figure out to fix the jumpiness, will maybe look into it again later if people report it.
auth/src/test/java/com/firebase/ui/auth/viewmodel/PhoneProviderResponseHandlerTest.java
Show resolved
Hide resolved
targetField = f; | ||
} | ||
} | ||
|
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.
Nit: extra space & I would name f "field" instead
auth/src/test/java/com/firebase/ui/auth/viewmodel/PhoneProviderResponseHandlerTest.java
Show resolved
Hide resolved
@@ -92,6 +102,16 @@ public int getPositionForSection(int index) { | |||
|
|||
@Override | |||
public int getSectionForPosition(int position) { | |||
if (sections == null) { |
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.
I tested this, and it seems a bit jumpy. If you scroll all the way down, and go back up, it immediately jumps to a quarter of the way up. If you can fix that, it'd be perfect. Otherwise, still a big improvement over what we had.
@@ -92,6 +102,16 @@ public int getPositionForSection(int index) { | |||
|
|||
@Override | |||
public int getSectionForPosition(int position) { | |||
if (sections == null) { |
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.
The scrollbar also doesn't show up unless we start scrolling - is that the intended behavior?
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.
Going to approve since it's mostly just nits.
Fixes:
Other changes:
.gradle
files to make Android Studio pick up the build easier. I got a new computer so I was able to experience the first-timer pain.