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.

Sign up for GitHub

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

Jump to bottom

fix(ios): allow TableViewRow proxy to return correct view #12225

Merged
merged 7 commits into from
Nov 3, 2020
Merged

fix(ios): allow TableViewRow proxy to return correct view #12225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
08a5296
fix(ios): allow TableViewRow proxy to return correct view
garymathews Oct 27, 2020
00e6578
test(ios): re-enable row rect test
garymathews Oct 27, 2020
c5fe5b2
fix(ios): use TiUIView for row container
garymathews Oct 27, 2020
f033ccb
Merge branch 'master' into TIMOB-27935
garymathews Oct 27, 2020
1ffc37b
Merge branch 'master' into TIMOB-27935
garymathews Oct 28, 2020
3f3d595
Merge branch 'master' into TIMOB-27935
garymathews Oct 29, 2020
209e6e0
Merge branch 'master' into TIMOB-27935
Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions iphone/Classes/TiUITableViewRowProxy.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
#ifdef TI_USE_AUTOLAYOUT
@interface TiUITableViewRowContainer : TiLayoutView
#else
@interface TiUITableViewRowContainer : UIView
@interface TiUITableViewRowContainer : TiUIView
#endif
{
TiProxy *hitTarget;
Expand Down Expand Up @@ -528,7 +528,7 @@ - (TiProxy *)parentForBubbling

- (UIView *)view
{
return nil;
return rowContainerView;
Copy link
Contributor

Choose a reason for hiding this comment

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

@garymathews TiUITableViewRowContainer is subclass of UIView. Ideally it should be subclass of TiUIView. It may crash at some point because all functions and properties available in TiUIView is not available with TiUITableViewRowContainer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should view enforce TiUIView?

- (TiUIView *)view

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've updated the PR, TiUITableViewRowContainer is now a subclass of TiUIView

Copy link
Contributor

Choose a reason for hiding this comment

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

@garymathews I am not sure if it will work. I guess it may cause layout issues. 1. Can you test long tableview (at least some tableview rows goes out of screen) with different contents (to verify reusability of cells).
2. Need to test a bit more around TableViewRow properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my testing, everything works as it should with no layout or event issues. I don't think using TiUIView is necessary, but there's no harm in maintaining the change.

All of the tests are passing, including the problematic tests for TIMOB-28148 and TIMOB-27935.

TEST CASE
NOTE: I ran through more tests than this, but this provides a nice base to test more properties and complex views.
const win = Ti.UI.createWindow();
const tableData = [];

for (let i = 1; i <= 100; i++) {
	const row = Ti.UI.createTableViewRow();
	const label = Ti.UI.createLabel({
		color: 'black',
		text: `Row #${i}`,
		height: 50
	});

	row.add(label);
	tableData.push(row);
}

const tableView = Ti.UI.createTableView({ data: tableData });

tableView.addEventListener('click', e => {
	const row = tableData[e.index];
	const message = `Clicked row ${e.index + 1}\n\nrect.width: ${row.rect.width}\nrect.height: ${row.rect.height}`;

	// NOTE: `borderRadius` on TableViewRow has no effect on iOS.
	row.borderRadius = 20;
	row.backgroundColor = 'red';

	Ti.API.info(message);
	alert(message);
});
tableView.addEventListener('longpress', e => {
	const message = `Long-pressed row ${e.index + 1}`;

	Ti.API.info(message);
	alert(message);
});

const restartButton = Ti.UI.createButton({
	title: 'Restart',
	bottom: 5,
	right: 5
});
restartButton.addEventListener('click', _ => {
	Ti.App._restart();
});

win.add([ tableView, restartButton ]);
win.open();

We can notify QE to run through more TableView tests.

}

//Private method : For internal use only
Expand Down
3 changes: 1 addition & 2 deletions tests/Resources/ti.ui.tableview.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1481,8 +1481,7 @@ describe('Titanium.UI.TableView', function () {
win.open();
});

it.iosBroken('row#rect', function (finish) {
// FIXME: TIMOB-27935
it('row#rect', function (finish) {
if (isCI && utilities.isMacOS()) { // FIXME: On macOS CI (maybe < 10.15.6?), times out! Does app need explicit focus added?
return finish(); // FIXME: skip when we move to official mocha package
}
Expand Down