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-6392] Android fix: Ti.UI.Button text values with too many dis… #9299

Merged
merged 6 commits into from Nov 13, 2017

Conversation

maggieaxway
Copy link
Contributor

@maggieaxway maggieaxway commented Aug 10, 2017

…playable characters draw improperly

[TIMOB-6392]- Android fix: Ti.UI.Button text values with too many
displayable characters draw improperly

JIRA: https://jira.appcelerator.org/browse/TIMOB-6392

Test case:

var window = Ti.UI.createWindow({ layout: "vertical" });
window.add(Ti.UI.createButton(
{
	title: "Single Line Button",
	top: "5%",
}));
window.add(Ti.UI.createButton(
{
	title: "Multiline\nButton",
	top: "5%",
}));
window.add(Ti.UI.createButton(
{
	title: "Single Line Truncated Button",
	top: "5%",
	width: "50%",
}));
window.add(Ti.UI.createButton(
{
	title: "Multiline Button\nThis is the next very long line.",
	top: "5%",
	width: "50%",
}));
window.add(Ti.UI.createButton(
{
	title: "Very long multiline button with word wrapping.",
	top: "5%",
	width: "50%",
}));
window.add(Ti.UI.createButton(
{
	title: "Very long Multiline button with word wrapping.",
	top: "5%",
	width: "50%",
	height: "5%",
}));
window.open();

…playable characters draw improperly

[TIMOB-6392]- Android fix: Ti.UI.Button text values with too many
displayable characters draw improperly
@jquick-axway
Copy link
Contributor

@maggieaxway, I did a quick test on iOS and it turn out we don't support multiline buttons on that platform. That is, there is no line wrapping and it ignores newline '\n' characters. It's always forced to be single-line. iOS also always applies ellipsis in the middle of the text as well.

So, for parity with iOS, let's do the following in the TiUIButton constructor:

  • Force to single line via setMaxLines(1).
  • Use TextUtils.TruncateAt.MIDDLE ellipsis mode instead. (Matches iOS behavior.)

This also means you don't need to do the above dynamically anymore. So, you can get rid of the onGlobalLayout() listener. This is also a breaking change and should only go into Titanium 7.0 (don't back-port it), meaning that multiline button support will be lost. But it's better to add new button properties to both Android and iOS to enable such a feature to make it portable.

For parity with iOS:

Force to set single line
Use TextUtils.TruncateAt.MIDDLE ellipsis mode
}
});
btn.setEllipsize(TextUtils.TruncateAt.MIDDLE);
btn.setMaxLines(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move these 2 lines into the TiUIButton constructor please?
(Before the setNativeView(btn) call.)

This way these setting will always be applied. This is in case the "title" property is not set upon button creation and is set afterwards instead.

Other than that, yeah, it should be this simple. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right! I didn't notice it. Appreciate your caution!

Move changes to constructor to make sure always applied
Copy link
Contributor

@jquick-axway jquick-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: Pass

@ssjsamir ssjsamir self-requested a review August 17, 2017 18:18
@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Nov 9, 2017

FR Passed.

For longer button text on android the text are shortened using ellipses like IOS.

Studio Ver: 4.10.0.201709271713
SDK Ver: 7.0.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.10
Appc CLI: 6.3.0
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.7
Node Ver: 7.10.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 6P --- Android 8.0.0
⇨ google Nexus 5 --- Android 6.0.1

@build build added the android label Nov 9, 2017
@build
Copy link
Contributor

build commented Nov 9, 2017

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Generated by 🚫 dangerJS

@lokeshchdhry
Copy link
Contributor

@maggieaxway , Jenkins is asking for unit tests before merge. Can you please add it.

@jquick-axway
Copy link
Contributor

I don't think there is a good way to automate testing of this particular case. You have to eyeball the text and make sure it's showing ellipsis.

@maggieaxway
Copy link
Contributor Author

@lokeshchdhry There's no crash before or after the changes, it just meant to display properly. So I agree with Josh it doesn't make sense to make auto test.

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Nov 10, 2017

@sgtcoolguy , Can you please merge this as according to @maggieaxway no unit tests can't be added. Thanks!

@sgtcoolguy sgtcoolguy merged commit c7ce84b into tidev:master Nov 13, 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

7 participants