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-12196] initial ListView implementation #3947

Merged
merged 7 commits into from Mar 12, 2013
Merged

[TIMOB-12196] initial ListView implementation #3947

merged 7 commits into from Mar 12, 2013

Conversation

mstepanov
Copy link
Contributor

TIMOB-12196

Test case attached to TIMOB-13014

@ghost ghost assigned vishalduggal Mar 9, 2013
@vishalduggal
Copy link
Contributor

First test I ran was to see how default templates work
Heres the test case http://pastebin.com/JiNuhkn5

Observed results:
No way to set the height on a tableView row
image property of the LIST_ITEM_TEMPLATE_DEFAULT must be local. remote does not work (not documented)

Continuing FR

@vishalduggal
Copy link
Contributor

Previous discussions had indicated that auto height of the listitem will not be supported. Do not see that documented anywhere so will consider it a bug until specified in spec.

@vishalduggal
Copy link
Contributor

Stopping FR now. Looks like there is a separate ticket for eventing and test case creation is in progress. Is this PR to only test the template mechanism? Will resume once there is clarity on the scope of this PR.

@mstepanov
Copy link
Contributor Author

@vishalduggal ready for CR+FR. Test case attached to TIMOB-13014

@@ -0,0 +1,25 @@
/**
* Appcelerator Titanium Mobile
* Copyright (c) 2009-2013 by Appcelerator, Inc. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

All new files are copyright 2013 only.

Also missing ifdef statements for all new files in the ListModule

@vishalduggal
Copy link
Contributor

rowHeight property of listView is specified as a V2 property, but that seems to be the only way of controlling row heights in this implementation.
Also this is supposed to be the default row height but the actual rowHeight itself must be returned from the tableView delegate methods which are not implemented

id propertiesValue = [item objectForKey:@"properties"];
NSDictionary *properties = ([propertiesValue isKindOfClass:[NSDictionary class]]) ? propertiesValue : nil;
BOOL allowsSelection = [TiUtils boolValue:[properties objectForKey:@"allowsSelection"] def:YES];
return allowsSelection ? indexPath : nil;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you return nil from here, is the 'didSelectRowAtIndexPath' method called. If not,this is a change in behavior from tableView and will fail FR since the spec does not state that setting this to false will prevent itemClick event.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also if the user taps the accessory button instead of trying to select the row, the itemClick will fire, which is a discrepancy in behavior.

@vishalduggal
Copy link
Contributor

One thing I noticed in the test cases is the setting of color and font for title properties for all built in templates although the spec says that it is supported only for default template. Can we update spec or is this hidden iOS goods?

@vishalduggal
Copy link
Contributor

image property does not seem to work with Ti.UI.LIST_ITEM_TEMPLATE_CONTACTS. Bug or limitation? No documentation in spec.


+ (UIView*)titleViewForText:(NSString*)text inTable:(UITableView *)tableView footer:(BOOL)footer
{
CGSize maxSize = CGSizeMake(320, 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be using the tableView bounds here? Especially for width since 320 is an iPhone constant.

@srahim
Copy link
Contributor

srahim commented Mar 12, 2013

Functionally Reviewed and approved for now.

srahim added a commit that referenced this pull request Mar 12, 2013
[TIMOB-12196] initial ListView implementation
@srahim srahim merged commit 80fc994 into tidev:master Mar 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants