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-24841] iOS: Add Ti.UI.AlertDialog.tintColor #9150

Merged
merged 7 commits into from Jun 26, 2017

Conversation

hansemannn
Copy link
Collaborator

@hansemannn hansemannn added this to the 6.2.0 milestone Jun 16, 2017
@sgtcoolguy
Copy link
Contributor

Please add a test for this property on iOS.

Copy https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/ti.ui.alertdialog.test.js into tests/Resources and add a test that runs on iOS that creates an alert dialog with this property set and changed (basically copy the 'title' test but have it set/change/validate the tintColor)

@hansemannn
Copy link
Collaborator Author

@sgtcoolguy There is no unit-test that could properly test it, only pseudo-tests like setting the kroll-property. PR's like this one would rather need a UI-test, but I can still add a test, sure.

@sgtcoolguy
Copy link
Contributor

sgtcoolguy commented Jun 20, 2017

You'd be surprised, but just some simple tests to set/change/get simple properties like this have actually turned up bugs. I understand that really this needs some sort of functional UI test to verify the proper portion of the dialog gets colored appropriately, but that's beyond what we can do automated right now.

We could also long-term improve the test suite to validate properties handling specific types (i.e. that it handles all the same possible values for colors as other color properties do: hex, names, etc; or that it can handle being given an invalid type such as a boolean or number in an expected way) to make this test (and others like it) more useful.

@hansemannn
Copy link
Collaborator Author

hansemannn commented Jun 20, 2017

Did I get you correctly that I can copy the whole file into tests/ and the build will automatically move it to the titanium-mobile-mocha-suite repo? Basically, I'd add this:

// Skip on Android, since it's an iOS-only property
(utilities.isAndroid() ? it.skip : it)('tintColor', function () {
	var bar = Ti.UI.createAlertDialog({
		tintColor: 'red'
	});
	
	// Check getter
	should(bar.tintColor).be.a.String;
	should(bar.getTintColor).be.a.Function;
	should(bar.tintColor).eql('red');
	should(bar.getTintColor()).eql('red');
	
	// Set new value
	bar.tintColor = '#f00';
	
	// Check getter again
	should(bar.tintColor).be.a.String;
	should(bar.getTintColor).be.a.Function;
	should(bar.tintColor).eql('#f00');
	should(bar.getTintColor()).eql('#f00');
});

@sgtcoolguy
Copy link
Contributor

Yes, copy the whole file and make the edits to add the new test into it. We take the tests/ folder as an override and copy it over top the titanium-mobile-mocha-suite when generating the unit test project.

@hansemannn
Copy link
Collaborator Author

@sgtcoolguy thank you, added in 073b293.

});

// Skip on Android, since it's an iOS-only property
(utilities.isAndroid() ? it.skip : it)('tintColor', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to skip for any non-iOS platforms (mobilweb, windows).
(utilities.isIOS() ? it : it.skip)('tintColor', function () {


// Check getter again
should(bar.tintColor).be.a.String;
should(bar.getTintColor).be.a.Function;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need to check this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought it may be possible that the generated getter could be manipulated so the test would fail. Adjusting.

should(bar.tintColor).be.a.String;
should(bar.getTintColor).be.a.Function;
should(bar.tintColor).eql('#f00');
should(bar.getTintColor()).eql('#f00');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a setter method we should check exists, and can be called?

It sounds simple/dumb, but I swear on Android I saw a case where the property setter and the actual getter method were "disconnected" where you could set different values for each and they didn't update the same underlying property.

Copy link
Contributor

@vijaysingh-axway vijaysingh-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 and FT passed.

Copy link

@htbryant94 htbryant94 left a comment

Choose a reason for hiding this comment

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

Approved! tintColor property for AlertDialog is now supported. Tested with both string & hex color values, as well as checking for errors if incorrect argument values are specified.

@htbryant94 htbryant94 merged commit abca714 into tidev:master Jun 26, 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