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

fix(ios): set tintcolor of UIBarButtonItem #12245

Merged
merged 12 commits into from Nov 20, 2020

Conversation

vijaysingh-axway
Copy link
Contributor

@build build added this to the 9.3.0 milestone Nov 4, 2020
@build build requested a review from a team November 4, 2020 01:25
@build build added the ios label Nov 4, 2020
@build
Copy link
Contributor

build commented Nov 4, 2020

Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 8576 tests are passing.
(There are 679 skipped tests not included in that total)

Generated by 🚫 dangerJS against a2d718c

tests/Resources/ti.ui.window.test.js Outdated Show resolved Hide resolved
tests/Resources/ti.ui.window.test.js Outdated Show resolved Hide resolved
tests/Resources/ti.ui.window.test.js Outdated Show resolved Hide resolved
tests/Resources/ti.ui.window.test.js Outdated Show resolved Hide resolved
tests/Resources/ti.ui.window.test.js Outdated Show resolved Hide resolved
tests/Resources/ti.ui.window.test.js Outdated Show resolved Hide resolved
tests/Resources/ti.ui.window.test.js Outdated Show resolved Hide resolved
iphone/Classes/TiUINavBarButton.m Outdated Show resolved Hide resolved
try {
should(rootWindow.leftNavButton).be.an.Object();
should(rootWindow.rightNavButton).be.an.Object();
should(win).not.matchImage(nonColorButtonImage, 0); // Navbutton without color should be different with a colored one
Copy link
Contributor

Choose a reason for hiding this comment

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

The test I had in mind, was to simply change the color for a navbutton and verify the image matches expectations for this PR (namely that the text color is the one you assigned). I suppose it can be useful to verify that one given color: 'green' doesn't look the same as one without color property specified, but you can get to the same place by simply having one test verify the "default" no-color image, and another verify the green color button. Or simply a single test with multiple buttons: one assigned a color, one assigned a tintColor, one with neither assigned and verify that image.

Copy link
Contributor Author

@vijaysingh-axway vijaysingh-axway Nov 5, 2020

Choose a reason for hiding this comment

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

@sgtcoolguy If you are talking about matching against hardcoded image, snapshot does not match on iPhone SE (2nd generation) simulator and iPhone 11 simulator even we use 'px' (e.g. 400px) to set width and height of navigation window. Reason looks nav button titles are a bit different in both simulator.
So I compared screenshots with colored and non colored nav bar button and it should not match.
I had tried -

	it.ios('.leftNavButton and .rightNavButton  with color', finish => {
		const rightButton = Ti.UI.createButton({
			title: 'Right',
			tintColor: 'red',
			color: 'green',
		});

		const leftButton = Ti.UI.createButton({
			title: 'Left',
			color: 'red'
		});

		const rootWindow = Ti.UI.createWindow({
			backgroundColor: 'white',
			leftNavButton: leftButton,
			rightNavButton: rightButton,
		});

		win = Ti.UI.createNavigationWindow({
			height: '400px',
			width: '400px',
			window: rootWindow
		});

		win.open();

		rootWindow.addEventListener('postlayout', function postlayout() {
			rootWindow.removeEventListener('postlayout', postlayout);
			setTimeout(function () {
				try {
					should(rootWindow.leftNavButton).be.an.Object();
					should(rootWindow.rightNavButton).be.an.Object();
					should(win).matchImage('snapshots/navButtonWithColor.png');
				} catch (e) {
					return finish(e);
				}
				finish();
			}, 1000);
		});
	});

But screenshots from iPhone 11 Pro max and iPhone SE (2nd generation) simulator didn't match. Am I missing anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know since I don't have the images to look at manually. I assume that despite the use of pixel dimensions some things may matter based on screen dpi, so you'd have to use the density in the filename of the image and produce copies for varying densities?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this locally on this PR, the issue is indeed a scale/density issue whereby the button text is sized differently on different density screens (since text/fonts are always in typographical points on iOS: https://appcelerator.github.io/titanium-docs/api/structs/font.html#fontsize)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. You are right. Different scale images should be added. Added 2x and 3x images. I think no need of scale 1x image as we do not support those devices.

@ssekhri
Copy link

ssekhri commented Nov 19, 2020

FR Passed
Verified on:
Mac OS: 10.15.4
SDK: 9.3.0.v20201116174003
Appc CLI: 8.1.1
JDK: 11.0.6
Node: 12.16.1
Studio: 6.0.0.202005141803
Xcode: 12.0.1
iPhone 7Plus(v14.0), iPhone X(v13.6.1), iOS sim 14.0

@sgtcoolguy sgtcoolguy modified the milestones: 9.3.0, 10.0.0 Nov 20, 2020
@sgtcoolguy sgtcoolguy merged commit 1a5307e into tidev:master Nov 20, 2020
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