TIMOB-9386: Android: Implement conditional horizontal layout wrapping #2368
Conversation
…e into timob-9386
…layouts with wrap
|
Added additional drillbit tests for horizontal layout with no wrap. This should be ready for CR/FR. |
|
@ayeung - we need to document this behavior as part of this PR as well. |
|
Added docs for new horizontal layout with wrap behavior |
|
Review in progress |
| @@ -497,17 +499,26 @@ protected void onLayout(boolean changed, int l, int t, int r, int b) | |||
|
|
|||
| // Try to calculate width/height from pins, and default to measured width/height. We have to do this in | |||
There was a problem hiding this comment.
The comment should be changed to reflect the code changes
|
Code reviewed and single comment left. Functional review run and first test in ticket looks ok but the behavior looks odd to me for the second test case. WIll follow up with Allen regarding expected behavior. |
|
MobileWeb horizontal warp PR #2408 |
|
functional test failed when running test attached in the comments. layout of the 3rd row is not correct - already spoke to Allen about this and he is looking into it. Hold for fix. |
| On Android, the first row is centered vertically in the parent view, and | ||
| successive rows are placed below the first row as on iOS. However, the `top` | ||
| and `bottom` values are interpreted relative to the parent view. | ||
| When the wrap property is disabled, the behavior is identical to a vertical layout, |
There was a problem hiding this comment.
property can not be disabled. Should read when wrap property is set to false.
|
IOS and mobileWeb have now defined the property as |
| successive rows are placed below the first row as on iOS. However, the `top` | ||
| and `bottom` values are interpreted relative to the parent view. | ||
| When the wrap property is disabled, the behavior is identical to a vertical layout, | ||
| except children are laid out horizontally from left to right, _in rows_. |
There was a problem hiding this comment.
Shouldn't be in rows when wrap is false. It's a single row. Something like this maybe?
If the `wrap` property is false, the behavior is more equivalent to a vertical layout. Children are laid or horizontally from left to right in a single row. The `left` and `right` properties are used as padding between the children, and the `top` and `bottom` properties are used to position the children vertically.
|
I have concerns with the test cases attached in TIMOB-9386. If this PR is merged in the present state then please clarify the questions raised in the ticket. |
|
Updated PR to have fill behavior for test case 3, as per talk with vishal. @vishalduggal could you take a look at this again to see if I'm missing anything? |
| exists in the current row, it is wrapped to a new row. The height of each row | ||
| is equal to the maximum height of the children in that row. | ||
| * `horizontal`. Horizontal layouts have different behavior depending on whether wrapping | ||
| is enabled. Wrapping is enabled by default (the `wrap` property is `true`). | ||
|
|
There was a problem hiding this comment.
horizontalWrap property
| the children in that row. | ||
|
|
||
| Wrapping behavior is available on iOS, Android and Mobile Web (Release 2.1.0 and later). | ||
| When the wrap property is set to true, the first row is placed at the top of the |
There was a problem hiding this comment.
horizontalWrap property
|
Doc needs to be updated for the new property name. Have iOS and Mobile Web also updated this property name? Ran validate and docgen, everything was clean except for these points. |
|
Docs reviewed for IOS. ACCEPTED |
|
Docs reviewed, validate and docgen passed. I have concerns about the TIMOB-9575 fix, stated in that bug. In my opinion the iOS and Mobile Web fix is inconsistent with our other layout logic, and we "fixed" parity by making Android behave the same way. |
|
Arthur - Can you please explain the inconsistency and what should be the correct behavior? |
|
In composite layout, a child with no top/bottom positioning pins will be centered. In a horizontal layout with no wrap, a child with no top/bottom positioning pins will be centered. In horizontal layout with wrap, a child with no top/bottom positioning pins is top-aligned in its row. I don't understand why we would do this. And if the interpretation of top/bottom pins is completely different in "wrap" mode, then we need to document how they behave. |
|
Code reviewed and functional test passed. docgen and validate passed. Accepted |
TIMOB-9386: Android: Implement conditional horizontal layout wrapping
|
Forgot to approve officially, but talked to Opie and Max and I think my concerns were unfounded and we're doing the right thing. Accepted. |
https://jira.appcelerator.org/browse/TIMOB-9386
This PR adds a flag to enable/disable wrapping in horizontal layout. It also changes the behavior of horizontal layout with wrap to bring it in parity with iOS.
For testing, please run the new drillbit tests, and also run the test cases attached to TIMOB-9386 and TIMOB-9575.
This ticket also resolves https://jira.appcelerator.org/browse/TIMOB-8980.