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

fix(android): use of right property in child of horizontal layout #12182

Closed
wants to merge 1 commit into from

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Oct 15, 2020

  • Correct behavior of right property when applied to last child in horizontal layout for parity with iOS
TEST CASE
const win = Ti.UI.createWindow({
	backgroundColor: 'gray'
});
const row = Ti.UI.createView({
	layout: 'horizontal',
	backgroundColor: 'white',
	width: Ti.UI.FILL,
	height: 50
});
const view_a = Ti.UI.createView({
	backgroundColor: 'green',
	left: 50,
	// right: 50,
	width: 50,
	height: 50
});
const view_b = Ti.UI.createView({
	backgroundColor: 'red',
	// left: 50,
	right: 50,
	width: 50,
	height: 50
});

row.add([ view_a, view_b ]);
win.add(row);
win.open();
  • Last child of horizontal layout should align to the right.
  • In all other cases (where both left and right properties are defined), view should stack from left with expected padding.

JIRA Ticket

@garymathews garymathews added this to the 9.3.0 milestone Oct 15, 2020
@build build requested a review from a team October 15, 2020 19:14
@build
Copy link
Contributor

build commented Oct 15, 2020

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 7655 tests are passing.
(There are 742 skipped tests not included in that total)

Generated by 🚫 dangerJS against 3e581d2

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

I think this calls for a simple snapshot test using the snippet from the PR body (although to avoid having to generate different variants by density, you should use pixel sizes (instead of numbers without units).

@jquick-axway
Copy link
Contributor

So, I don't think we should make this change.

It turns out Android and iOS' behavior (before this PR) is already consistent. In a "horizontal" layout, setting the "right" property to right-align a view within a container is not supported on Android or iOS. Likewise, you cannot do the same with a "vertical" layout and use the "bottom" property to bottom align the view on Android and iOS.

See the following scripts I attached to TIMOB-28197.

  • HorizontalPinAlignTest.js
  • VerticalPinAlignTest.js

Neither of the above scripts will do what the ticket expects on Android or iOS, but that's not necessarily wrong either. You can easily do this with a "composite" layout. I'm leaning towards not changing the layout behavior.

(I know this PR is trying to resolve a ListView issue I found last week. We might need to take a different approach.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants