-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
tests/Resources/ti.ui.window.test.js
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
FR Passed |
https://jira.appcelerator.org/browse/TIMOB-28211