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

[TIMOB-24792] Android: Fix horizontal layout #9123

Merged
merged 2 commits into from Jun 9, 2017

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Jun 7, 2017

  • Fix condition for calculating right bound of Titanium.UI.View
  • Titanium.UI.ListItem should ignore the layout property
TEST CASE #1
var win = Ti.UI.createWindow({title: 'TIMOB-24277', backgroundColor: 'gray', layout: 'horizontal'}),
    a = Ti.UI.createView({
        height: 100,
        width: 100,
        borderColor: 'red',
        borderWidth: 5,
        backgroundColor: 'blue',
        right: 10
    }),
    b = Ti.UI.createView({
    	height: 100,
    	width: 100,
    	borderColor: 'purple',
    	borderWidth: 5,
    	backgroundColor: 'orange',
    	right: 10
    });
 
win.add([a, b]);
win.open();
TEST CASE #2
var win = Ti.UI.createWindow(),
	items = [],
	template = {
		properties: {
			accessoryType: Ti.UI.LIST_ACCESSORY_TYPE_NONE,
			backgroundColor: 'white',
			height: Ti.UI.SIZE,
			layout: 'horizontal'
		},
		events: {
			click: function(e) {
				alert(e.source.customProperty);
			}
		},
		childTemplates:
		[
			{
				type: 'Ti.UI.Button',
				bindId: 'bindButton',
				properties: {
					left: 10,
					width: 100,
					height: 40,
					borderRadius: 20,
					borderWidth: 1,
					borderColor: '#dedede',
					backgroundColor: 'green'
				},
			},
			{
				type : 'Ti.UI.Button',
				bindId : 'bindButton2',
				properties : {
					right: 10,
					width: 100,
					height: 40,
					borderRadius: 20,
					borderWidth: 1,
					borderColor: '#dedede',
					backgroundColor: 'red'
				}
			}
		]
	},
	listView = Ti.UI.createListView({templates: {'template': template}}),
	section = Ti.UI.createListSection();

for(var i = 1; i <= 10; i++){
	items.push({
		template: 'template',
		bindButton: {
		   title: 'button a ' + i,
		   customProperty: i
		},
		bindButton2: {
		   title: 'button b ' + i,
		   customProperty: i
		},
		properties: {
		   itemId: i
		}
	});
}

section.setItems(items);
listView.sections = [section];

win.add(listView);
win.open();
NOTE: this requires a thorough test to make sure no other layout issues are introduced

JIRA Ticket

Copy link
Contributor

@mukherjee2 mukherjee2 left a comment

Choose a reason for hiding this comment

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

Node Version: 6.10.3
NPM Version: 3.10.10
Mac OS: 10.12.4
Appc CLI: 6.2.2
Appc CLI NPM: 4.2.9
Titanium SDK version:master (6.2.0) pr/9124
Appcelerator Studio, build: 4.9.0.201705302345
Xcode 8.3.2

Passed FR. In addition to the test case above, I tried changing other property values. Based on my testing, the bug is considered fixed.

@garymathews garymathews changed the title [TIMOB-24277] Android: Fix horizontal layout [TIMOB-24792] Android: Fix horizontal layout Jun 8, 2017
@jquick-axway
Copy link
Contributor

jquick-axway commented Jun 9, 2017

This is definitely the right solution.

I checked our "ListItem" documentation and it doesn't support a "layout" property. I double-checked iOS and we definitely ignore the ListItem.layout property on that platform. So, this whole time, the TIMOB-24277 bug we were originally trying to fix was really just a platform inconsistency. Android was respecting the given horizontal layout property when it shouldn't have. If a developer needs to use a horizontal or vertical layout within a ListView row, then the portable way to do it on Android and iOS is to set up a view container with a layout and add child views to that container. I've tested that and it works.

@garymathews, great job identifying the issue!

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@m1ga
Copy link
Contributor

m1ga commented Jun 9, 2017

@jquick-axway I didn't test this PR yet, but have you tried the code I've mentioned here: TIMOB-24277 with this fix?

@garymathews
Copy link
Contributor Author

@m1ga Yes, it works as expected

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

5 participants