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
[TIMOB-25360] Android: ScrollView enhancements #9513
Conversation
- [TIMOB-25197] Added RefreshControl support to vertical and horizontal ScrollViews. - [TIMOB-8898] Added Ti.UI.FIll and Ti.UI.SIZE support to ScrollView "contentWidth" and "contentHeight" properties. - [TIMOB-25360] ScrollView child views that use Ti.UI.FILL should do a 100% fill like iOS if contentWidth/Height properties are not set. - [TIMOB-25377] ScrollView child percent size/position should be relative to container size, like iOS. * Was relative to ScrollView's scrollable content area. - [TIMOB-12153] ListView/TableView height set to Ti.UI.SIZE will return wrong height when put inside a ScrollView. * Was returning a height equivalent to Ti.UI.FILL, which was limited to size of parent view or window. - Improved ScrollView measurement/layout performance: * Was always doing a 2-pass measurement, which would cause issues with percentage calculations. * Now only does a 1-pass mearurement.
FR Passed. Used the test code in the tickets to test :
Studio Ver: 4.10.0.201709271713 |
* Determines if this view's onMeasure() has been called. | ||
* @return Returns true if this view's onMeasure() has been called. Returns false if not. | ||
*/ | ||
public boolean wasOnMeasureCalled() |
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.
Maybe call this getOnMeasureCalled
?
for (int index = getChildCount() - 1; index >= 0; index--) { | ||
// Fetch the next child. | ||
View child = getChildAt(index); | ||
if (child == 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.
Could combine if
statements:
if (child == null || child.getVisibility() == View.GONE) {
continue;
}
if (newValue instanceof RefreshControlProxy) { | ||
Log.w(TAG, REFRESH_CONTROL_NOT_SUPPORTED_MESSAGE); | ||
View nativeView = getNativeView(); | ||
if (newValue == 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.
This could be simplified to:
if (nativeView instanceof TiSwipeRefreshLayout) {
RefreshControlProxy.unassignFrom((TiSwipeRefreshLayout) nativeView);
if (newValue instanceof RefreshControlProxy) {
((RefreshControlProxy) newValue).assignTo((TiSwipeRefreshLayout) nativeView);
}
} else {
Log.e(TAG, "Invalid value assigned to property '" + key + "'. Must be of type 'RefreshControl'.");
}
Updated PR based on feedback. |
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.
CR: PASS
@lokeshchdhry, this PR can be merged now. |
Merging this PR as of now, but the regressions for scrollview : |
JIRA:
Summary:
Vertical ScrollView RefreshControl Test:
Horizontal ScrollView RefreshControl Test:
ScrollView Percent Test:
Vertical ScrollView Fill Test:
(See the JIRA case's attached "VerticalScrollFill-Good.gif" for example of expected result.)
Horizontal ScrollView Fill Test:
(See the JIRA case's attached "HorizontalScrollFill-Good.gif" for example of expected result.)
ScrollView "contentHeight" Fill Test:
ScrollView TableView Size Test:
(Note that the TableView is not scrollable. Instead, the TableView is using Ti.UI.SIZE for height and you're scrolling the ScrollView instead.)