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-23436]:ListView Control's Height not setting in iOS #8937

Merged
merged 8 commits into from Jul 20, 2017

Conversation

vijaysingh-axway
Copy link
Contributor

@@ -1762,7 +1762,7 @@ - (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPa
}
if (TiDimensionIsDip(height)) {
return height.value;
} else if (TiDimensionIsAutoSize(height)) {
} else if (TiDimensionIsAutoSize(height) || TiDimensionIsUndefined(height)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test-case works for me. But I am wondering if we shouldn't include this as well:

    } else if (TiDimensionIsAuto(height) || TiDimensionIsAutoSize(height) || TiDimensionIsUndefined(height)) {

And I also noticed that the template do has a height and width specified, so it shouldn't go until that line, can you check that before approving? Thx! Good work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While setting Ti.UI.auto for row height , it gives undefined. Rather I found that for Ti.UI.Fill check similar condition should applied . Updated check with TiDimensionIsAutoFill(height).
Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mhh, I'd be really curious if checking for size and fill is a good idea. Pretty sure it breaks something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can not think of any use case where it can break something. Are you ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansemannn Ping.

@hansemannn
Copy link
Collaborator

hansemannn commented May 31, 2017

Please add more details why exactly this change fixes this issue. Also, add UI tests for all layout-specific use-cases for all if-statements available in the source. I am trying to avoid a major layout regression like in TIMOB-24753, so we better check this more detailed.

@hansemannn hansemannn added this to the 6.2.0 milestone Jul 18, 2017
Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Was able to set the ListView Control's height without any issues.

Test Steps

  • Downloaded the SDK Build from this PR
  • Created a new Titanium project
  • Copied the test case from the comments in https://jira.appcelerator.org/browse/TIMOB-23436
  • Ran the application
  • Was able to see the ListView height change
  • Changed the ListView height in the code
  • Was able to see the new height

Test Environment
Appcelerator Command-Line Interface, version 6.2.2
iPhone 6S (10.0)
iPhone 6S Simulator(10.2)
Operating System Name: Mac OS X El Capitan
Operating System Version: 10.11.6
Node.js Version: 6.10.1
Xcode: 8.2
Appcelerator Studio: 4.9.0.201705251638

@ssjsamir ssjsamir merged commit 29130f8 into tidev:master Jul 20, 2017
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